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: updates for running on Windows #2681

Closed
wants to merge 5 commits into from

Conversation

webstech
Copy link
Contributor

This patch series contains updates to support the ci tests running on Windows.

  1. ci: run docker tests on Windows
    The latest docker engine api needs some more config specs to allow an open port (equivalent to -p on the command line).

  2. ci: windows support

    • change the path separator in a test regex when running on Windows
    • turn off credential.helper in the git config in the credential tests. This will suppress pop-ups for the password on Windows systems.
  3. ci: use td.repleaseEsm instead of td.replace
    The replace functions are now the same as the other tests.

  4. ci: change package name to avoid collision
    There is a test-release package in the docker image at a higher release. A different package name will now be used.

  5. ci: serialize a plugin test
    The index test Plugins are called with expected values is now serialized to avoid possible concurrency problems. This matches the rest of the tests in index.test.js.

@webstech
Copy link
Contributor Author

Note: for the ci to run on Windows, changes are required to the quibble package, which is used for mocking by the testdouble package.

@webstech
Copy link
Contributor Author

@travi Can you please approve the workflow run? I would like to verify the docker related changes run (or need version checks added).

@webstech
Copy link
Contributor Author

This is in support of issue #2658 and pr #2659.

@travi
Copy link
Member

travi commented Jan 20, 2023

@webstech i kicked off the workflow run. sorry for being unavailable over the last week to provide feedback along the way. i expect to have some time available this afternoon to look more closely and give some feedback, but it may be a few hours yet.

i really appreciate you looking into this part. i do want to highlight that i dont see running our verification on windows as a blocker to getting your fixes merged with the other PR. i do think it would be valuable to test the new code paths with the existing test suites in some way so that we avoid introducing a similar regression in the future.

having things run on windows in ci would be great, but if that requires completely rethinking major parts of how the pieces work in the pipeline, it may not be worth the effort and maintenance costs. either way, i'll focus first on talking through any changes we need for your other PR to get merged and decisions about running ci on windows can be a separate decision for later.

@webstech
Copy link
Contributor Author

i kicked off the workflow run.

I saw a timeout message before the log became unavailable.

Using the log from my other PR that timed out I compared with a successful run locally. All the tests have run so the delay is in the termination. Guessing without proof it is shutting down the docker images in integration.test.js. I could add a timer to test.after.always() to:

  1. prove this is the problem
  2. terminate the wait

I did not see any open issue for this.

@travi
Copy link
Member

travi commented Jan 21, 2023

there are issues with our verification currently that seem to cause them to timeout and not exit correctly. a recent update to our test framework, ava, mentioned a fixed to issues that sound a lot like what we're seeing. the problems seemed to start for us after the update that introduced that "fix", so i attempted to revert today, but that doesnt seem to be a fix either.

in short, i dont think the test issues youve seen with out pipeline are at all related to your changes or running on windows. i dont know of a fix yet, and we may need to wait for more fixes to be released in ava, but that shouldnt stop us from getting your fixes in (but might slow us down in getting successful builds).

@webstech
Copy link
Contributor Author

i dont know of a fix yet, and we may need to wait for more fixes to be released in ava

Sorry for jumping in with suggestions without knowing the history.

Thanks for reviewing the changes.

@webstech webstech marked this pull request as ready for review January 21, 2023 05:00
@travi
Copy link
Member

travi commented Jan 21, 2023

Sorry for jumping in with suggestions without knowing the history.

no need to apologize. your investigation is appreciated, just want to save you effort that is likely unrelated

The index test `Plugins are called with expected values` is now
serialized to avoid possible concurrency problems.  This matches the rest of
the tests in `index.test.js`.

Signed-off-by: Chris. Webster <chris@webstech.net>
There is a test-release package in the docker image at a higher release.
A different package name will now be used.

Signed-off-by: Chris. Webster <chris@webstech.net>
The replace functions are now the same as the other tests.

Signed-off-by: Chris. Webster <chris@webstech.net>
1. change the path separator in a test regex when running on Windows
2. turn off `credential.helper` in the git config in the credential
tests.  This will suppress pop-ups for the password on Windows systems.

Signed-off-by: Chris. Webster <chris@webstech.net>
The latest docker engine api needs some more config specs to allow an
open port (equivalent to -p on the command line).

Signed-off-by: Chris. Webster <chris@webstech.net>
@webstech
Copy link
Contributor Author

webstech commented Feb 21, 2023

@travi The latest run fails due to timing with a duplicate repo name in integration.test.js. Running with a changed name got past the failure:

test("ESM Plugin with named exports", async (t) => {
  const packageName = "foo-secret";

I can open a PR to change the name but maybe you have a preference that is more descriptive.🙂

Running locally, I still had a couple of errors:

message: `Command failed with exit code 128: git checkout -b master␊
    warning: unable to access 'C:/Users/chris/.gitconfig': Permission denied␊
    fatal: unknown error occurred while reading the configuration files`,

Repeated runs got past the errors. Not sure if this is a simple timing error or something that needs to be managed in the tests by controlling the user level git config location using vars. Do you have any thoughts on that?

EDIT: Other PRs are having the same problem with the duplicate repo name. The other test is Hide sensitive environment variable values from the logs. Sometimes the error shows up on that test.

@webstech
Copy link
Contributor Author

Just noting that there were Windows related changes to the test tools that have been merged in. This included the testdouble, quibble and teenytest packages. Quibble issues were seen in the CI here but the others had changes for their own CI to run on Windows.

@webstech
Copy link
Contributor Author

there are issues with our verification currently that seem to cause them to timeout and not exit correctly.

Looking at the git config access error some more, I saw that the message Rejected promise returned by test was also showing up. I had to interrupt the jobs to get the final error output. So, here are a couple of tracebacks.

  › makeError (file:///node_modules/execa/lib/error.js:59:11)
  › handlePromise (file:///node_modules/execa/index.js:119:26)
  › async gitShallowClone (file:///test/helpers/git-utils.js:177:3)
  › async Module.createRepo (file:///test/helpers/gitbox.js:71:15)
  › async file:///test/integration.test.js:417:43

and

  › makeError (file:///node_modules/execa/lib/error.js:59:11)
  › handlePromise (file:///node_modules/execa/index.js:119:26)
  › async gitCheckout (file:///test/helpers/git-utils.js:130:3)
  › async initBareRepo (file:///test/helpers/git-utils.js:76:3)
  › async gitRepo (file:///test/helpers/git-utils.js:51:5)
  › async addChannelMacro (file:///test/index.test.js:791:34)

Maybe an execa issue?

@travi
Copy link
Member

travi commented Feb 22, 2023

there are still some test issues on the mainline branch that i havent had time to investigate yet. it sounds like you may have identified one of the issues. if you want to look into things further, it would probably be worth investigating those issues on the mainline separately from this PR. resolving those issues could make it more clear what the real status of these changes is.

@webstech
Copy link
Contributor Author

Converted to draft pending closure. Pushing patches separately based on new research.

@webstech
Copy link
Contributor Author

webstech commented Mar 4, 2023

Closing this PR. All changes have been done in separate PRs except this update to integration.test.js:

ci: use td.repleaseEsm instead of td.replace
The replace functions are now the same as the other tests.

@webstech webstech closed this Mar 4, 2023
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

2 participants