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.

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

ci: run tests on Windows #553

Closed
wants to merge 15 commits into from
Closed

ci: run tests on Windows #553

wants to merge 15 commits into from

Conversation

G-Rath
Copy link
Collaborator

@G-Rath G-Rath commented Sep 26, 2023

Resolves #603

@codecov-commenter
Copy link

codecov-commenter commented Sep 26, 2023

Codecov Report

Merging #553 (ce6d89c) into main (a85d675) will decrease coverage by 0.05%.
The diff coverage is 65.85%.

@@            Coverage Diff             @@
##             main     #553      +/-   ##
==========================================
- Coverage   78.73%   78.69%   -0.05%     
==========================================
  Files          79       80       +1     
  Lines        5742     5782      +40     
==========================================
+ Hits         4521     4550      +29     
- Misses       1033     1043      +10     
- Partials      188      189       +1     
Files Coverage Δ
internal/output/sarif.go 90.57% <100.00%> (+0.15%) ⬆️
internal/sourceanalysis/go.go 67.28% <100.00%> (+1.27%) ⬆️
internal/url/url.go 57.57% <57.57%> (ø)

... and 1 file with indirect coverage changes

@G-Rath G-Rath changed the title ci: run tests on ubuntu, macos, and windows ci: run tests on Windows Oct 3, 2023
@another-rex
Copy link
Collaborator

I think I fixed the (first) govulncheck problem with 734094c, the error it revealed I have to guess is because of line endings (I'm not sure why the line endings would change, but the +12 differences matches up with the line number/num of new lines). I think we just have to have two different test values for different platforms?

@another-rex
Copy link
Collaborator

Oh I know why it has windows line endings, it's the same reason the SARIF test is failing, the github action git checkout is configured to automatically change line endings to the windows version when checking out on windows, just configuring it to not do that will fix both the SARIF error and the govulncheck test error

@G-Rath
Copy link
Collaborator Author

G-Rath commented Oct 13, 2023

just configuring it to not do that will fix both the SARIF error and the govulncheck test error

I don't think doing that is the end of the world, but also is this something Go should actually be handling 🤔

I've inlined json.MarshalIndent and had it output \r if on Windows and the tests are now looking better (still not passing but for different reasons) - I'm not sure if it is valid for Go to just always output \n or if it should internally be sorting this; looking around though it also sounds like Go has decided this our problem to deal with not theirs...

another-rex pushed a commit that referenced this pull request Nov 2, 2023
Cherry-picked from #553

---

It's required for testing against Windows because it has a different
error message, but it's also just a good overall change and landing it
separately removes ~25 files from the main PR 😅
another-rex pushed a commit that referenced this pull request Nov 9, 2023
Turns out that file -> url translation on Windows is busted, and that
this is a hard problem that Go has an internal util for that has not yet
been made public - I've done what apparently a number of other packages
have done which is copying that helper into here and hoping one day it
actually becomes public 😢

Note that until #646 is landed, there is no way to actually verify this
is fixing the problem - #553 shows the result of both PRs being merged.
another-rex pushed a commit that referenced this pull request Nov 21, 2023
Unsurprisingly this has required a bunch of tests to be updated to
handle slightly different variations in file path handling - this
eventually resulted in me implementing an actual internal snapshot
testing package but I've not included that in here since its sizable on
its own; so please keep that in mind when reviewing . (see
https://github.com/G-Rath/osv-scanner/commit/1273da79e2e26a18d663da482dc5f09258e15c51
for a sneakpeek on what the snapshot-based testing looks like)

~Note that is failing because file -> url path translation is actually
busted; I've opened #645 to fix this and you can see the passing CI when
both of these changes are merged in #553~

Resolves #603
Resolves #553
@G-Rath G-Rath deleted the run-tests-on-os branch January 22, 2024 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Run tests against Windows
3 participants