Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

*: fix windows tests #2353

Merged
merged 9 commits into from Feb 10, 2022
Merged

*: fix windows tests #2353

merged 9 commits into from Feb 10, 2022

Conversation

AnnaShaleva
Copy link
Member

@AnnaShaleva AnnaShaleva commented Feb 8, 2022 •
edited

Close #2269, close #2277.

Part of #2268 is also fixed, but dealing with NEO-GO-VM > ←[J←[2K is still in progress.

@codecov
Copy link

codecov bot commented Feb 8, 2022 •
edited

Codecov Report

Merging #2353 (0760e54) into master (b5afe24) will increase coverage by 0.11%.
The diff coverage is 34.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2353      +/-   ##
==========================================
+ Coverage   84.15%   84.27%   +0.11%     
==========================================
  Files         287      287              
  Lines       35319    35412      +93     
==========================================
+ Hits        29724    29842     +118     
+ Misses       4261     4221      -40     
- Partials     1334     1349      +15
Impacted Files Coverage Δ
cli/server/server.go 79.95% <30.00%> (-6.96%) ⬇️
pkg/compiler/analysis.go 96.10% <100.00%> (+0.81%) ⬆️
pkg/vm/stackitem/item.go 91.06% <0.00%> (-0.34%) ⬇️
pkg/network/server.go 74.08% <0.00%> (-0.10%) ⬇️
cli/wallet/nep17.go 72.12% <0.00%> (+0.02%) ⬆️
pkg/rpc/client/helper.go 46.87% <0.00%> (+0.15%) ⬆️
pkg/rpc/server/server.go 76.91% <0.00%> (+0.53%) ⬆️
pkg/core/storage/memcached_store.go 91.07% <0.00%> (+1.40%) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 62602af...0760e54. Read the comment docs.

roman-khimov
roman-khimov reviewed Feb 8, 2022
View reviewed changes
cli/server/server.go Show resolved Hide resolved
cli/server/server.go Outdated Show resolved Hide resolved
cli/server/server.go Outdated Show resolved Hide resolved
pkg/vm/cli/cli_test.go Outdated Show resolved Hide resolved
fyrchik
fyrchik reviewed Feb 9, 2022
View reviewed changes
cli/server_test.go Outdated
require.Eventually(t, func() bool {
line, err = e.Out.ReadString('\n')
if err != nil && err != io.EOF {
t.Fatalf(fmt.Sprintf("unexpected error while reading CLI output: %s", err))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to use fmt.Sprintf with Fatalf.

pkg/vm/cli/cli_test.go Outdated Show resolved Hide resolved
@AnnaShaleva AnnaShaleva force-pushed the fix-win-tests branch 9 times, most recently from 5a30c88 to f3c9540 Compare February 9, 2022 17:18
@AnnaShaleva
cli: add temp workaround to handle absolute log path for Windows
59fdb69 
Fix #2269.
@AnnaShaleva
cli: ensure opened sinks are properly closed
2736d62 
zap never closes open sinks except its own tests. This behaviour
prevents TestHandleLoggingParams from successful cleanup because
temp log output file can't be closed due to the following error:
```
TempDir RemoveAll cleanup: remove C:\\Users\\Anna\\AppData\\Local\\Temp\\TestHandleLoggingParams_debug5796883
33\\001\\file.log: The process cannot access the file because it is being used by another process.
```

So this tremendous cludge is made mosetly for our testing code.
It is not for concurrent usage (we don't have cases of
multithreaded access to logger output sink).
@AnnaShaleva
cli: ensure chain is properly stopped after chain-related commands
8ff7cd8 
Blockchain occupies resources (e.g. it opens log files for DB, etc.)
on creation and running. We need to release these resources if something
goes wrong during execution chain-related commands.

This commit solves the following problem on Windows:
```
--- FAIL: TestServerStart (0.32s)
    --- FAIL: TestServerStart/stateroot_service_is_on_&&_StateRootInHeader=true (0.04s)
        testing.go:894: TempDir RemoveAll cleanup: remove C:\Users\Anna\AppData\Local\Temp\TestServerStart_stateroot_service_is_on_&&_StateRootInHeader=true460557297\001\neogotestchain\000001.log: The process cannot access the file because it is being used by another process.
    --- FAIL: TestServerStart/invalid_Oracle_config (0.03s)
        testing.go:894: TempDir RemoveAll cleanup: remove C:\Users\Anna\AppData\Local\Temp\TestServerStart_invalid_Oracle_config810064028\001\neogotestchain\000001.log: The process cannot access the file because it is being used by another process.
    --- FAIL: TestServerStart/invalid_consensus_config (0.04s)
        testing.go:894: TempDir RemoveAll cleanup: remove C:\Users\Anna\AppData\Local\Temp\TestServerStart_invalid_consensus_config217270091\001\neogotestchain\000001.log: The process cannot access the file because it is being used by another process.
    --- FAIL: TestServerStart/invalid_Notary_config (0.07s)
        --- FAIL: TestServerStart/invalid_Notary_config/malformed_config (0.04s)
            testing.go:894: TempDir RemoveAll cleanup: remove C:\Users\Anna\AppData\Local\Temp\TestServerStart_invalid_Notary_config_malformed_config754934830\001\neogotestchain\000001.log: The process cannot access the file because it is being used by another process.
        --- FAIL: TestServerStart/invalid_Notary_config/invalid_wallet (0.03s)
            testing.go:894: TempDir RemoveAll cleanup: remove C:\Users\Anna\AppData\Local\Temp\TestServerStart_invalid_Notary_config_invalid_wallet934249397\001\neogotestchain\000001.log: The process cannot access the file because it is being used by another process.
    --- FAIL: TestServerStart/good (0.11s)
        testing.go:894: TempDir RemoveAll cleanup: remove C:\Users\Anna\AppData\Local\Temp\TestServerStart_good596150160\001\neogotestchain\000001.log: The process cannot access the file because it is being used by another process.
```

This commit also unifies blockchain and services releasing code.
@AnnaShaleva AnnaShaleva force-pushed the fix-win-tests branch 6 times, most recently from 72f6750 to feedce8 Compare February 10, 2022 10:58
AnnaShaleva and others added 2 commits February 10, 2022 18:04
@AnnaShaleva
cli: do not run test server on windows
d73f95e 
Currently we can't properly stop running server on Windows and SIGHUP
is also not supported. This leads to occupied resources and failed
test cleanup:
```
--- FAIL: TestServerStart (0.35s)
    --- FAIL: TestServerStart/good (0.10s)
        testing.go:894: TempDir RemoveAll cleanup: remove C:\Users\Anna\AppData\Local\Temp\TestServerStart_good337747932\001\neogotestchain\000001.log:
The process cannot access the file because it is being used by another process.
2022-02-08T14:11:20.959+0300    INFO    persisted to disk       {"blocks": 0, "keys": 112, "headerHeight": 0, "blockHeight": 0, "took": "10.0049ms"}
```
@AnnaShaleva
vm: refactor VM CLI tests as they take too long to pass
a987206 
These tests are slow on Windows, so refactor them a bit and avoid the following
error:
```
--- FAIL: TestRunWithDifferentArguments (4.01s)
        cli_test.go:96:
                Error Trace:    cli_test.go:96
                                                                                cli_test.go:321
                Error:          command took too long time
                Test:           TestRunWithDifferentArguments
```
@AnnaShaleva AnnaShaleva marked this pull request as ready for review February 10, 2022 15:19
@AnnaShaleva
compiler: use full paths for debug info files if relative can't be co…
0de4c0f 
…nstructed

Firstly introduced in 9871dc8. It's OK
if relative path can't be constructed e.g. for interop dependencies. Otherwice
leads to failing tests on Windows with the following error:
```
2022-02-09T09:39:31.3307626Z panic: Rel: can't make D:\a\neo-go\neo-go\pkg\interop\neogointernal\syscall.go relative to C:\Users\RUNNER~1\AppData\Local\Temp\TestContractInitAndCompile1998984267\001\testcontract [recovered]
2022-02-09T09:39:31.3308830Z 	panic: Rel: can't make D:\a\neo-go\neo-go\pkg\interop\neogointernal\syscall.go relative to C:\Users\RUNNER~1\AppData\Local\Temp\TestContractInitAndCompile1998984267\001\testcontract
2022-02-09T09:39:31.3309285Z
2022-02-09T09:39:31.3309390Z goroutine 302 [running]:
2022-02-09T09:39:31.3309725Z testing.tRunner.func1.2({0x14dbfe0, 0xc00051bdb0})
2022-02-09T09:39:31.3310187Z 	C:/hostedtoolcache/windows/go/1.17.6/x64/src/testing/testing.go:1209 +0x36c
2022-02-09T09:39:31.3310538Z testing.tRunner.func1()
2022-02-09T09:39:31.3310937Z 	C:/hostedtoolcache/windows/go/1.17.6/x64/src/testing/testing.go:1212 +0x3b6
2022-02-09T09:39:31.3311285Z panic({0x14dbfe0, 0xc00051bdb0})
2022-02-09T09:39:31.3311689Z 	C:/hostedtoolcache/windows/go/1.17.6/x64/src/runtime/panic.go:1047 +0x266
2022-02-09T09:39:31.3312319Z github.com/nspcc-dev/neo-go/pkg/compiler.(*codegen).fillDocumentInfo.func1(0xc0002d0b40)
2022-02-09T09:39:31.3312885Z 	D:/a/neo-go/neo-go/pkg/compiler/analysis.go:240 +0x34d
2022-02-09T09:39:31.3313397Z go/token.(*FileSet).Iterate(0xc0000a69c0, 0xc0004898b0)
2022-02-09T09:39:31.3313839Z 	C:/hostedtoolcache/windows/go/1.17.6/x64/src/go/token/position.go:463 +0xde
2022-02-09T09:39:31.3314404Z github.com/nspcc-dev/neo-go/pkg/compiler.(*codegen).fillDocumentInfo(0xc0002e4d20)
2022-02-09T09:39:31.3314947Z 	D:/a/neo-go/neo-go/pkg/compiler/analysis.go:236 +0x97
2022-02-09T09:39:31.3315530Z github.com/nspcc-dev/neo-go/pkg/compiler.(*codegen).compile(0xc0002e4d20, 0xc00009a690, 0xc000101540)
2022-02-09T09:39:31.3316064Z 	D:/a/neo-go/neo-go/pkg/compiler/codegen.go:2088 +0x192
2022-02-09T09:39:31.3316563Z github.com/nspcc-dev/neo-go/pkg/compiler.codeGen(0xc00009a690)
2022-02-09T09:39:31.3317066Z 	D:/a/neo-go/neo-go/pkg/compiler/codegen.go:2167 +0x7cd
2022-02-09T09:39:31.3317679Z github.com/nspcc-dev/neo-go/pkg/compiler.CompileWithOptions({0xc0000435e0, 0x62}, {0x0, 0x0}, 0xc0004bcf70)
2022-02-09T09:39:31.3318233Z 	D:/a/neo-go/neo-go/pkg/compiler/compiler.go:225 +0xde
2022-02-09T09:39:31.3318804Z github.com/nspcc-dev/neo-go/pkg/compiler.CompileAndSave({0xc0000435e0, 0x62}, 0xc0004bcf70)
2022-02-09T09:39:31.3319353Z 	D:/a/neo-go/neo-go/pkg/compiler/compiler.go:241 +0x3a5
2022-02-09T09:39:31.3319889Z github.com/nspcc-dev/neo-go/cli/smartcontract.contractCompile(0xc0004e42c0)
2022-02-09T09:39:31.3320456Z 	D:/a/neo-go/neo-go/cli/smartcontract/smart_contract.go:520 +0xc2d
2022-02-09T09:39:31.3320873Z github.com/urfave/cli.HandleAction({0x14b6fa0, 0x17b8d60}, 0x7)
2022-02-09T09:39:31.3321344Z 	C:/Users/runneradmin/go/pkg/mod/github.com/urfave/cli@v1.22.5/app.go:524 +0xf4
2022-02-09T09:39:31.3321931Z github.com/urfave/cli.Command.Run({{0x16460d2, 0x7}, {0x0, 0x0}, {0x0, 0x0, 0x0}, {0x166fe9e, 0x27}, {0x0, ...}, ...}, ...)
2022-02-09T09:39:31.3322443Z 	C:/Users/runneradmin/go/pkg/mod/github.com/urfave/cli@v1.22.5/command.go:173 +0xc09
2022-02-09T09:39:31.3322894Z github.com/urfave/cli.(*App).RunAsSubcommand(0xc0000b7340, 0xc0004e4000)
2022-02-09T09:39:31.3323375Z 	C:/Users/runneradmin/go/pkg/mod/github.com/urfave/cli@v1.22.5/app.go:405 +0x106c
2022-02-09T09:39:31.3323852Z github.com/urfave/cli.Command.startApp({{0x1647536, 0x8}, {0x0, 0x0}, {0x0, 0x0, 0x0}, {0x1671070, 0x28}, {0x0, ...}, ...}, ...)
2022-02-09T09:39:31.3324365Z 	C:/Users/runneradmin/go/pkg/mod/github.com/urfave/cli@v1.22.5/command.go:372 +0x108c
2022-02-09T09:39:31.3324838Z github.com/urfave/cli.Command.Run({{0x1647536, 0x8}, {0x0, 0x0}, {0x0, 0x0, 0x0}, {0x1671070, 0x28}, {0x0, ...}, ...}, ...)
2022-02-09T09:39:31.3325327Z 	C:/Users/runneradmin/go/pkg/mod/github.com/urfave/cli@v1.22.5/command.go:102 +0xe8c
2022-02-09T09:39:31.3325744Z github.com/urfave/cli.(*App).Run(0xc000323880, {0xc0004237a0, 0xb, 0x12})
2022-02-09T09:39:31.3326198Z 	C:/Users/runneradmin/go/pkg/mod/github.com/urfave/cli@v1.22.5/app.go:277 +0xa8c
2022-02-09T09:39:31.3326770Z github.com/nspcc-dev/neo-go/cli.(*executor).run(0xc0000709c0, {0xc0004237a0, 0xb, 0x12})
2022-02-09T09:39:31.3327275Z 	D:/a/neo-go/neo-go/cli/executor_test.go:273 +0x3a5
2022-02-09T09:39:31.3327812Z github.com/nspcc-dev/neo-go/cli.(*executor).Run(0x14aa020, 0xc000493950, {0xc0004237a0, 0xb, 0x12})
2022-02-09T09:39:31.3328324Z 	D:/a/neo-go/neo-go/cli/executor_test.go:263 +0x10f
2022-02-09T09:39:31.3328844Z github.com/nspcc-dev/neo-go/cli.TestContractInitAndCompile(0xc0001d69c0)
2022-02-09T09:39:31.3329377Z 	D:/a/neo-go/neo-go/cli/contract_test.go:155 +0x147f
2022-02-09T09:39:31.3329694Z testing.tRunner(0xc0001d69c0, 0x17b8be8)
2022-02-09T09:39:31.3330123Z 	C:/hostedtoolcache/windows/go/1.17.6/x64/src/testing/testing.go:1259 +0x230
2022-02-09T09:39:31.3330470Z created by testing.(*T).Run
2022-02-09T09:39:31.3330862Z 	C:/hostedtoolcache/windows/go/1.17.6/x64/src/testing/testing.go:1306 +0x727
2022-02-09T09:39:31.3331325Z FAIL	github.com/nspcc-dev/neo-go/cli	5.552s

```
@AnnaShaleva
network: tune waiting limits in tests
9bbd94d 
Some tests are failing on Windows due to slow runners with errors like the following:
```
2022-02-09T17:11:20.3127016Z     --- FAIL: TestGetData/transaction (1.82s)
2022-02-09T17:11:20.3127385Z         server_test.go:500:
2022-02-09T17:11:20.3127878Z             	Error Trace:	server_test.go:500
2022-02-09T17:11:20.3128533Z             	            				server_test.go:520
2022-02-09T17:11:20.3128978Z             	Error:      	Condition never satisfied
2022-02-09T17:11:20.3129479Z             	Test:       	TestGetData/transaction
```
@AnnaShaleva
github: fix publishing job for WSC image
2cf15ea 
Close #2277.
@AnnaShaleva
dockerfile: reduce build time for WSC-based image
0760e54 
Turns out that caching the golang:windowsservercore-ltsc2022 image between
GithubAction workflow runs is a bad idea because `docker load` command still takes
too long to load image from cached archive (~9-10min on standard windows runner).
And after that runner still needs to build the neo-go image itself.

However, standard GA windows runner is supplied with prefetched latest
mcr.microsoft.com/windows/servercore:ltsc2022 image, so using it costs almost
nothing. Thus, the other approach is implemented: we use standard
mcr.microsoft.com/windows/servercore:ltsc2022 image as both build-base and
final-base. Then we install all required tools for building neo-go manually
on build-base image (these tools are git and go 1.17). Compared to the first
approach, the publishing job (included build and publish to DockerHub)
takes ~7-8min to finish.
@roman-khimov roman-khimov merged commit acd348c into master Feb 10, 2022
@roman-khimov roman-khimov deleted the fix-win-tests branch February 10, 2022 18:57
roman-khimov added a commit that referenced this pull request Mar 9, 2022
@roman-khimov
Revert "Makefile: add unsupported to docker windows image tag and b…
8a94cf7 
…in version"

This reverts commit 6d28e75. It's no longer
relevant after #2353 and #2365.
@roman-khimov roman-khimov mentioned this pull request Mar 9, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fyrchik fyrchik fyrchik left review comments

@roman-khimov roman-khimov roman-khimov approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

.github: adjust Push images to DockerHub workflow after Windows tests are fixed Fix logging on Windows
3 participants
@AnnaShaleva @fyrchik @roman-khimov