fix: extend test timeout for windows

[dougal83]

Give windows extra time to complete tests
Fixes #4425

fix: cleanup tilde-path-app post test …
ensure test app is remove afterward
Fixes #4652

Checklist

• npm test passes on your machine
• New tests added or existing tests modified to cover all changes
• Code conforms with the style guide

[dougal83]

@derdeka If you have time, could you please run tests against this PR?

[dougal83]

@agnes512 Does this look reasonable? I had to do this for CI test but I'm not sure how that works. I've also dropped a clause to compare the sandbox path from L266.

[bajtos]

Hmm, I find this suspicious. On the next line, we are running a cleanup script in the sandbox directory:

https://github.com/strongloop/loopback-next/blob/9aba52e59c80830dd0871c4a087b90ec2aa21db2/packages/cli/test/integration/generators/app.integration.js#L304

Does the behavior of run-clean depend on the current working directory? If it does not, then perhaps we can get rid of this first process.chdir entirely, and keep the last chdir(cwd) only?

   after(function() {
    // Increase the timeout to accommodate slow CI build machines
    // eslint-disable-next-line no-invalid-this
    this.timeout(30 * 1000);

    build.clean(['node', 'run-clean', sandbox]);
    process.chdir(cwd);
  });

[dougal83]

This appears to be a special case for tilde check. We are using sandbox that is outside the code's project root. Not setting the process.chdir gives for example: Skipping /home/dev/workspace/loopback-next/sandbox/tilde-path-app as it is not inside the project root directory.

[bajtos]

@dougal83 thank you for continuing the investigation of test failures on Windows! ❤️

The commit changing test timeouts LTGM, let's discuss how to best fix the tilde test.

[bajtos]

Hmm, I find this suspicious. On the next line, we are running a cleanup script in the sandbox directory:

https://github.com/strongloop/loopback-next/blob/9aba52e59c80830dd0871c4a087b90ec2aa21db2/packages/cli/test/integration/generators/app.integration.js#L304

Does the behavior of run-clean depend on the current working directory? If it does not, then perhaps we can get rid of this first process.chdir entirely, and keep the last chdir(cwd) only?

   after(function() {
    // Increase the timeout to accommodate slow CI build machines
    // eslint-disable-next-line no-invalid-this
    this.timeout(30 * 1000);

    build.clean(['node', 'run-clean', sandbox]);
    process.chdir(cwd);
  });

[derdeka]

The changed timeouts LGTM and fixed failed tests for me.

Before this change:
3088 passing (5m)
13 pending
4 failing

With your change:
3092 passing (5m)
13 pending

The changes to directory changing looks strange to me and seems unrelated. Maybe you can split this into a seperate PR as this needs more discussion or should be refactored at all.

[dougal83]

npm ERR! 429 Too Many Requests - GET https://registry.npmjs.org/@commitlint/cli/-/cli-8.3.5.tgz

Usually Cloudflare? Will manually restart test later.

[bajtos]

LGTM 👍

The chdir fix is not pretty, but as long as it works, I think it's best to land it now and then clean it up later (if at all).

@agnes512 AFAICT from git blame, the tilde test was introduced by you as part of #3252. Can you please review the proposed changes and check if they preserve the intent of your test?

[dougal83]

The chdir fix is not pretty, but as long as it works, I think it's best to land it now and then clean it up later (if at all).

Amen. 🙏

@bajtos approved

[agnes512]

LGTM, thank you for fixing it!