fix: ensure unique sandbox directories for boot tests #5747

raymondfeng · 2020-06-16T00:06:40Z

Extracted from #5744 for ease of review.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
API Documentation in code was updated
Documentation in /docs/site was updated
Affected artifact templates in packages/cli were updated
Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

bajtos

Thank you for extracting this change in to a smaller pull request, it's much easier to review 👏

~~The changes are reasonable, I'd like to discuss where to put the ENV-based behavior.~~ <-- that was for #5744 🙈

Please get feedback from code owners (@jannyHou) too.

bajtos · 2020-06-16T14:14:57Z

packages/boot/src/__tests__/acceptance/application-metadata.booter.acceptance.ts

+    const appJsFile = resolve(__dirname, '../fixtures/application.js');
+    let appJs = fs.readFileSync(appJsFile, 'utf-8');
+    // Adjust the relative path for `import`
+    appJs = appJs.replace('../..', '../../..');


Can we extract this logic into sandbox.copyFile please? IIRC, copyFile is already detecting JS files and updating sourcemap URL to accommodate the new location, it would be great if we could do the same for import paths.

If it's too work to detect which paths to update, then perhaps copyFile can accept a new options to specify the mapping? Example usage:

await sandbox.copyFile( resolve(__dirname, '../fixtures/application.js'), 'dist/application.js', { transform: text => text.replace('../..', '../../..'), }, });

WDYT?

Maybe we should defer it. The current copyFile does not read the source file and we don't even know if the source is a text or binary file.

For context, here is the current copyFile implementation, you are right that we are not reading the source file.

https://github.com/strongloop/loopback-next/blob/862072b82ad88ec4b248e0b243dda58b53640f94/packages/testlab/src/test-sandbox.ts#L135-L146

I have two concerns about your current version:

(1)
The code is a bit involved and at different level of abstraction when compared the surrounding lines.

I'd like to read getApp as a sequence of instructions for copying files, I don't want to be distracted with implementation details of transforming source file content into target.

await sandbox.copyFile(resolve(__dirname, '../fixtures/application.js'), { // Adjust the relative path for `import` transform: text => text.replace('../..', '../../..'), }); await sandbox.copyFile(resolve(__dirname, '../fixtures/package.json'));

(2)
The code modifying application.js is duplicated in two test suites. These two places can easily get out of sync and it's likely additional copies will be introduced in the future as more test files are added.

Can you at least extract the code dealing with application.js into a shared test helper please, to avoid duplication?

packages/boot/src/__tests__/acceptance/crud-rest.api-builder.acceptance.ts

bajtos

I am not entirely happy with the proposed version, but I guess we can improve it later.

bajtos · 2020-06-26T09:32:09Z

packages/boot/src/__tests__/acceptance/application-metadata.booter.acceptance.ts

+    const appJsFile = resolve(__dirname, '../fixtures/application.js');
+    let appJs = fs.readFileSync(appJsFile, 'utf-8');
+    // Adjust the relative path for `import`
+    appJs = appJs.replace('../..', '../../..');


For context, here is the current copyFile implementation, you are right that we are not reading the source file.

https://github.com/strongloop/loopback-next/blob/862072b82ad88ec4b248e0b243dda58b53640f94/packages/testlab/src/test-sandbox.ts#L135-L146

I have two concerns about your current version:

(1)
The code is a bit involved and at different level of abstraction when compared the surrounding lines.

I'd like to read getApp as a sequence of instructions for copying files, I don't want to be distracted with implementation details of transforming source file content into target.

await sandbox.copyFile(resolve(__dirname, '../fixtures/application.js'), { // Adjust the relative path for `import` transform: text => text.replace('../..', '../../..'), }); await sandbox.copyFile(resolve(__dirname, '../fixtures/package.json'));

(2)
The code modifying application.js is duplicated in two test suites. These two places can easily get out of sync and it's likely additional copies will be introduced in the future as more test files are added.

Can you at least extract the code dealing with application.js into a shared test helper please, to avoid duplication?

bajtos · 2020-06-26T09:34:13Z

packages/boot/src/__tests__/acceptance/controller.booter.acceptance.ts

-    } catch (err) {
-      console.log(`Stopping the app threw an error: ${err}`);
-    }
+    await app?.stop();


Should we check the state first, for consistency with other stopApp implementations?

Suggested change

await app?.stop();

if (app?.state === 'started') await app?.stop();

In this case, app.start() is called.

bajtos · 2020-06-26T09:49:34Z

@raymondfeng FYI, it looks like these improvements are not enough to fix all booter tests when executed in parallel.

Can you please rebase your patch on top of the latest master and run npm t -- -p to check yourself?

Here are the test failures I am seeing: https://gist.github.com/bajtos/c2f45df9c41384c710d3712186910661

bajtos · 2020-06-26T17:36:16Z

packages/boot/src/__tests__/acceptance/application-metadata.booter.acceptance.ts

+    const appJsFile = resolve(__dirname, '../fixtures/application.js');
+    let appJs = fs.readFileSync(appJsFile, 'utf-8');
+    // Adjust the relative path for `import`
+    appJs = appJs.replace('../..', '../../..');


IIUC, we are trying to fix the following line:

https://github.com/strongloop/loopback-next/blob/4e5f721ca666ab5047f4c36fbcb36a39b76966bb/packages/boot/src/__tests__/fixtures/application.ts#L10

An idea to consider: instead of changing the import/require path in the application file copied to sandbox, can we put a short index.js file into the directory where ../.. points to, to make require('../..') work again?

// index.js module.exports = require('../');

I prefer to use transform to make it clear - having a magic index.js makes it difficult to understand.

raymondfeng · 2020-06-26T17:40:48Z

@bajtos I added transform option for TestSandbox.copyFile.

raymondfeng · 2020-06-26T17:42:07Z

@raymondfeng FYI, it looks like these improvements are not enough to fix all booter tests when executed in parallel.
Can you please rebase your patch on top of the latest master and run npm t -- -p to check yourself?
Here are the test failures I am seeing: https://gist.github.com/bajtos/c2f45df9c41384c710d3712186910661

I cannot reproduce the issue. With this PR, npm t -- -p works for me.

raymondfeng requested a review from jannyHou as a code owner June 16, 2020 00:06

raymondfeng mentioned this pull request Jun 16, 2020

fix: clean up sandbox directories after tests #5744

Closed

7 tasks

bajtos reviewed Jun 16, 2020

View reviewed changes

raymondfeng requested a review from bajtos June 18, 2020 23:05

bajtos mentioned this pull request Jun 26, 2020

test: enable parallel execution of Mocha tests #5831

Merged

7 tasks

bajtos approved these changes Jun 26, 2020

View reviewed changes

bajtos reviewed Jun 26, 2020

View reviewed changes

raymondfeng added 3 commits June 26, 2020 10:37

feat(testlab): add transform option for sandbox copyFile

2f04b7e

fix(boot): use unique sandbox subdirs for testing

b8d0ca0

chore(boot): clean up stopApp

6ad5c9e

raymondfeng force-pushed the unique-sandbox-for-tests branch from 1bc25f5 to 6ad5c9e Compare June 26, 2020 17:38

raymondfeng requested a review from agnes512 as a code owner June 26, 2020 17:38

raymondfeng merged commit 225411c into master Jun 26, 2020

raymondfeng deleted the unique-sandbox-for-tests branch June 26, 2020 17:53

	await app?.stop();
	if (app?.state === 'started') await app?.stop();

fix: ensure unique sandbox directories for boot tests #5747

fix: ensure unique sandbox directories for boot tests #5747

Uh oh!

Conversation

raymondfeng commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

bajtos left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos Jun 16, 2020

Choose a reason for hiding this comment

Uh oh!

raymondfeng Jun 16, 2020

Choose a reason for hiding this comment

Uh oh!

bajtos Jun 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

bajtos Jun 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos Jun 26, 2020

Choose a reason for hiding this comment

Uh oh!

raymondfeng Jun 26, 2020

Choose a reason for hiding this comment

Uh oh!

bajtos commented Jun 26, 2020

Uh oh!

bajtos Jun 26, 2020

Choose a reason for hiding this comment

Uh oh!

raymondfeng Jun 26, 2020

Choose a reason for hiding this comment

Uh oh!

raymondfeng commented Jun 26, 2020

Uh oh!

raymondfeng commented Jun 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

raymondfeng commented Jun 16, 2020 •

edited

Loading

bajtos left a comment •

edited

Loading

bajtos Jun 26, 2020 •

edited

Loading

bajtos Jun 26, 2020 •

edited

Loading