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

[BUGFIX]: Fix errors while running yarn start #7146

Merged
merged 3 commits into from
May 10, 2020

Conversation

snewcomer
Copy link
Contributor

close #7072

Screen Shot 2020-04-26 at 9 26 59 PM

This PR allows a developer to execute yarn start and run the tests in the browser.

We add the /version file with some tag/sha information at build time iff EMBER_CLI_TEST_COMMAND is set by Node on the global variable process.env at runtime.

Another option is to be more specific in the if conditional by allowing only development or test. However, if this module is excluded from production, then this may be sufficient in its current state.

const options = this.getEmberDataConfig();
let compatVersion = options.compatWith;
let tree = version(compatVersion);
return this.debugTree(this._super.treeForAddon.call(this, tree), 'addon-output');
Copy link
Contributor

Choose a reason for hiding this comment

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

So this leaks the version module into our asset-size check because we now include it in production builds from unpublished-test-infra.

I don't think it legitimately affects end consumers, but it is frustrating to have it included in the check when it should not be.

There's probably a check we could do here, but it wouldn't just be "is production" since we test production output too. Maybe we check if the command is "serve" ? @rwjblue thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we now include it in production builds from unpublished-test-infra.

That is a great point! I assumed from the name of this package that it was not included in a published prod build; while also noticing it was a devDependency for various pkgs. Side question - is there a short explanation or module that you could point me to in order to understand why unpublished-test-infra would be included in the process of a production build?

Also, would an alternative fix involve something like process.env.EMBER_ENV?

fix Safari test issue

Co-authored-by: Chris Thoburn <[email protected]>
@runspired runspired force-pushed the sn/test-infra-failure branch from b9ce7a0 to 1274cfc Compare May 8, 2020 23:05
let compatVersion = options.compatWith;
let tree = merge([existingTree, version(compatVersion)]);

return this.debugTree(this._super.treeForAddonTestSupport.call(this, tree), 'addon-output');
Copy link
Member

Choose a reason for hiding this comment

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

Can you change the debug tree name (this isn't the addon tree any more)?

@runspired runspired merged commit bb34e6c into emberjs:master May 10, 2020
snewcomer added a commit to snewcomer/data that referenced this pull request May 10, 2020
* [BUGFIX]: Fix errors while running yarn start

fix Safari test issue

Co-authored-by: Chris Thoburn <[email protected]>

* fix lint

* Change debug name to test-support

Co-authored-by: Chris Thoburn <[email protected]>
igorT pushed a commit that referenced this pull request Jun 5, 2020
* [BUGFIX]: Fix errors while running yarn start

fix Safari test issue

Co-authored-by: Chris Thoburn <[email protected]>

* fix lint

* Change debug name to test-support

Co-authored-by: Chris Thoburn <[email protected]>
igorT pushed a commit that referenced this pull request Jun 5, 2020
* [BUGFIX]: Fix errors while running yarn start

fix Safari test issue

Co-authored-by: Chris Thoburn <[email protected]>

* fix lint

* Change debug name to test-support

Co-authored-by: Chris Thoburn <[email protected]>
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.

Running tests via yarn start fails
3 participants