ci: Add windows test runner #2033
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AndrewSisley
force-pushed
the
2032-windows-mac
branch
from
51e3b75
to
bfd614b
Compare
AndrewSisley
changed the title
AndrewSisley
force-pushed
the
2032-windows-mac
branch
from
92c1f89
to
f76e1b4
Compare
AndrewSisley
changed the title
AndrewSisley
force-pushed
the
2032-windows-mac
branch
from
f76e1b4
to
90f1aee
Compare
|
The errors in the tests seem to all be related to incorrect file paths. Should be easy to fix. |
shahzadlone
pushed a commit
that referenced
this pull request
Nov 13, 2023
## Relevant issue(s) Resolves #2034 ## Description Adds a mac (latest) test run to our test matrix. This only use the lightest of configurations we have, I think this will catch any/99% issues that we would otherwise miss. Windows has been broken out to a different ticket, as quite a lot of tests fail on windows and it will take more effort to get working: #2033
AndrewSisley
force-pushed
the
2032-windows-mac
branch
6 times, most recently
from
219d378
to
8a8c66c
Compare
islamaliev
reviewed
Nov 14, 2023
| @@ -8,6 +8,13 @@ | |||
| // by the Apache License, Version 2.0, included in the file | |||
| // licenses/APL.txt. | |||
|
|
|||
| // The badger GC seems to struggle a bit on windows, and this file rapidly spins up and discards | |||
| // databases resulting in out of memory and vlog cleanup issues, so we exclude this file from windows | |||
| // builds and and rely on the integration tests. | |||
There was a problem hiding this comment.
Cheers 😁
- Fix typo
There was a problem hiding this comment.
Was able to remove this restriction instead
AndrewSisley
force-pushed
the
2032-windows-mac
branch
7 times, most recently
from
2ea882b
to
08ad561
Compare
AndrewSisley
force-pushed
the
2032-windows-mac
branch
4 times, most recently
from
a4b36d3
to
cc97b47
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2033 +/- ##
===========================================
+ Coverage 73.78% 73.85% +0.07%
===========================================
Files 248 248
Lines 24801 24808 +7
===========================================
+ Hits 18298 18320 +22
+ Misses 5236 5226 -10
+ Partials 1267 1262 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
AndrewSisley
force-pushed
the
2032-windows-mac
branch
from
cc97b47
to
41f8fb7
Compare
AndrewSisley
force-pushed
the
2032-windows-mac
branch
2 times, most recently
from
456be88
to
63b9eb3
Compare
AndrewSisley
added
area/testing
The previous string asserted on was an os specific inner error that we wrapped with our own. When running on windows a different string was returned causing the test to fail. The test has been changed to assert on our custom error wrap, instead of the underlying message.
The previous string asserted on was an os specific inner error that we wrapped with our own. When running on windows a different string was returned causing the test to fail. The test has been changed to assert on our custom error wrap, instead of the underlying message.
And assert on our wrapped message instead of the inner. This is changing now because the inner is OS specific and was different on windows compared to mac/linux.
By leaving them unclosed they and their resources will remain active until all tests in the package have completed, resulting in a much larger resource cost than is needed. This set in particular resulted in the llvm race detector failing as the number of active routines exceeded the (hardcoded) limit of ~8100.
There is a race condition when closing the node, and the test looks odd - the name says it is expecting an error, yet it is not asserting for one, so I chose to remove it instead of removing the race.
Similar to the commit in the net package, this disposes of databases at the end of each test instead of keeping them all alive until the last test in the package completes (or the host runs out of memory, which is what the windows runner did).
Similar to the leaks in the net and db packages, this disposes of the test datstore at the end of each test. Most tests in this file already did so, but a handful didn't.
These tests do not pass on windows as the log files are still open when the test runner tries to delete them. I could not solve this quickly and it does not seem sensible to delay the windows test runner for the sake of a handful of log unit tests.
Credit goes to Fred for this fix.
Requested by Shahzad in a prior PR so that we are not reliant on github preserving it's current defaults. I also find it handy as it makes changing it to false for temp testing a fair bit easier.
AndrewSisley
force-pushed
the
2032-windows-mac
branch
from
63b9eb3
to
2bf25bb
Compare
shahzadlone
reviewed
Nov 20, 2023
shahzadlone
approved these changes
Nov 20, 2023
There was a problem hiding this comment.
Maybe @fredcarle can comment regarding the removed net test, all rest LGTM
This PR has idled for a while, can bring back/discuss the removed test post merge if need be. |
shahzadlone
pushed a commit
to shahzadlone/defradb
that referenced
this pull request
Jan 22, 2024
## Relevant issue(s) Resolves sourcenetwork#2032 ## Description Adds a windows test run to our test matrix.
shahzadlone
pushed a commit
to shahzadlone/defradb
that referenced
this pull request
Feb 23, 2024
## Relevant issue(s) Resolves sourcenetwork#2034 ## Description Adds a mac (latest) test run to our test matrix. This only use the lightest of configurations we have, I think this will catch any/99% issues that we would otherwise miss. Windows has been broken out to a different ticket, as quite a lot of tests fail on windows and it will take more effort to get working: sourcenetwork#2033
shahzadlone
pushed a commit
to shahzadlone/defradb
that referenced
this pull request
Feb 23, 2024
## Relevant issue(s) Resolves sourcenetwork#2032 ## Description Adds a windows test run to our test matrix.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Relevant issue(s)
Resolves #2032
Description
Adds a windows test run to our test matrix.
Strongly suggest reviewing commit by commit - I've tried to explain why a bunch of stuff I had to change has been changed in the commit bodies.
Notes
I can't spot anything obvious searching for this, and I've only seen it maybe twice/thrice out 30-40 runs, so at the moment I'm in favour of merging as-is, and then we can disable the cod-cov for windows runs whilst we fix/shelve it - but do let me know.