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

chore: add github actions workflow config #3078

Merged
merged 13 commits into from Aug 26, 2019
Merged

Conversation

shellscape
Copy link
Contributor

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

Description

This PR adds a Github Actions workflow config to the repo for Windows tests, to evaluate the feature.

@shellscape
Copy link
Contributor Author

shellscape commented Aug 23, 2019

@lukastaegert this has some failures that we don't see on AppVeyor. Please see https://github.com/rollup/rollup/pull/3078/checks?check_run_id=201673877. It looks like a whole slew of newline comparison problems. Do you have any insights about that?

@lukastaegert
Copy link
Member

Not really, but it seems like somehow it is injecting all the generated code with windows style \r\n linebreaks. Not sure how this could even happen!?

@shellscape
Copy link
Contributor Author

yeah it's bizarre to say the least. I think what I'll do is give those tests a platform check and see how it goes. one thing for certain - github actions for windows is 10x faster than appveyor

@lazka
Copy link

lazka commented Aug 24, 2019

Maybe git config --global core.autocrlf false before the checkout would help?

@shellscape
Copy link
Contributor Author

@lazka I'll definitely give that a shot, thank you for the suggestion!

@lukastaegert
Copy link
Member

Yes, @lazka's tip sounds spot-on with what we are observing, adding a platform would be unfortunate.

For reference, this is a list of the failing tests:
function/error-parse-json
form/deprecated/emit-asset
form/deprecated/emit-asset-file
chunking-form/deprecated/emit-asset
chunking-form/emit-file
chunking-form/preserve-modules-commonjs
chunking-form/preserve-modules-virtual-modules

@lukastaegert
Copy link
Member

Looking at what https://www.git-scm.com/book/en/v2/Customizing-Git-Git-Configuration#_code_core_autocrlf_code does, it seems to fit the issue exactly. For most form tests, the automatic addition of Windows line breaks is not an issue as it happens both to the source files as well as the _expected files. But for the tests in question, this conversion is only applied to some of the relevant sources or expectations and hence they fail.

@lukastaegert
Copy link
Member

Nice, I added the git repo config and now the tests run. Only remaining issue is that the "leak" test does not run on Windows due to reasons unclear, possibly because it cannot find node-gyp. Maybe the solution here is not to run the test on Windows as it really is enough to run it on one environment.

@lukastaegert
Copy link
Member

And yes, Github actions are really fast!

@codecov
Copy link

codecov bot commented Aug 25, 2019

Codecov Report

Merging #3078 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3078   +/-   ##
=======================================
  Coverage   88.81%   88.81%           
=======================================
  Files         165      165           
  Lines        5731     5731           
  Branches     1743     1743           
=======================================
  Hits         5090     5090           
  Misses        388      388           
  Partials      253      253

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 b1747e4...e533bee. Read the comment docs.

@shellscape
Copy link
Contributor Author

That test console output. Whew. I need to refresh my mocha knowledge to see if we can hide tests that succeed.

I'll make a change tonight to suppress the leak test from windows.

@shellscape
Copy link
Contributor Author

OK I think it's ready. Before we merge this we'll want to disconnect AppVeyor.

@lukastaegert
Copy link
Member

That test console output. Whew. I need to refresh my mocha knowledge to see if we can hide tests that succeed.

That's why the broken test are compiled again at the end. I guess the reason why they show a line for each test is so that you can see which test an unexpected log belongs to.

@lukastaegert
Copy link
Member

Maybe there is a way to have CircleCI compile a proper test report: https://circleci.com/docs/2.0/collect-test-data/#mochajs

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

Looks good from my side!

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.

None yet

4 participants