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

Run unit tests on Windows too #1099

Merged
merged 10 commits into from
Jul 12, 2022
Merged

Conversation

henrymercer
Copy link
Contributor

@henrymercer henrymercer commented Jun 15, 2022

This PR runs our unit tests on Windows, primarily to improve test coverage but also to support developing the Action on Windows machines. In doing so, we fix a couple of bugs.

For review: I spent a while looking at whether we could avoid the ugly hack in 6063828, however couldn't find anything. Do you have any thoughts?

Based on #1104.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

@henrymercer henrymercer force-pushed the henrymercer/run-unit-tests-on-windows branch 7 times, most recently from 612ac0e to 8f89ee5 Compare June 17, 2022 18:07
src/testing-utils.ts Fixed Show fixed Hide fixed
src/testing-utils.ts Fixed Show fixed Hide fixed
src/testing-utils.ts Fixed Show fixed Hide fixed
@henrymercer henrymercer force-pushed the henrymercer/run-unit-tests-on-windows branch 7 times, most recently from c6893e6 to 76ea1ff Compare June 17, 2022 21:07
@henrymercer henrymercer force-pushed the henrymercer/run-unit-tests-on-windows branch from 76ea1ff to abb2f8a Compare June 28, 2022 17:19
This decorator enabled us to use the functionality of the Actions
toolcache within the runner too.
Now that we've deleted the runner we no longer need it.
@henrymercer henrymercer force-pushed the henrymercer/run-unit-tests-on-windows branch 5 times, most recently from 41e985e to 9eed356 Compare June 29, 2022 09:07
@henrymercer henrymercer changed the base branch from main to henrymercer/remove-toolcache-decorator June 29, 2022 09:07
@henrymercer henrymercer force-pushed the henrymercer/run-unit-tests-on-windows branch from 9eed356 to a73b38a Compare June 29, 2022 09:07
@henrymercer henrymercer marked this pull request as ready for review June 29, 2022 09:30
@henrymercer henrymercer requested a review from a team as a code owner June 29, 2022 09:30
Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

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

This looks sensible. I just have a few questions inline so I can understand what is going on.

.github/workflows/pr-checks.yml Outdated Show resolved Hide resolved
Comment on lines +66 to +68
// Workaround an issue in tests where the case insensitivity of the `$PATH`
// environment variable on Windows isn't preserved, i.e. `process.env.PATH`
// is not the same as `process.env.Path`.
Copy link
Contributor

Choose a reason for hiding this comment

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

That's wild. When does this happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I spent a while looking into this, but couldn't track down what was going on. My suspicion is that this weirdness might have something to do with how we use an assignment to process.env to reset it at the end of every test to its value at the start of the test.

src/util.ts Show resolved Hide resolved
`toolcache.extractTar` currently falls over when `ACTIONS_TEMP` contains
a symlink, and the runner no longer exists, so it's unlikely our
customers would be running with temporary directories that contain
symlinks.
@henrymercer henrymercer force-pushed the henrymercer/run-unit-tests-on-windows branch from a73b38a to 130a51d Compare June 29, 2022 18:03
@henrymercer henrymercer force-pushed the henrymercer/remove-toolcache-decorator branch from 9953936 to b1742f8 Compare June 30, 2022 08:16
Base automatically changed from henrymercer/remove-toolcache-decorator to main July 11, 2022 16:54
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.

2 participants