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

enable ember-engine tests for 4.x versions as support has landed #1281

Merged

Conversation

void-mAlex
Copy link
Collaborator

@SergeAstapov
Copy link
Collaborator

@void-mAlex I believe we should add @ember/legacy-built-in-components to app dependencies for the ember-engines test

@void-mAlex
Copy link
Collaborator Author

@SergeAstapov should that be added for all versions or just the 4.x series?
I think I may have to split out the tests if it's only 4.x
for 3.28 the components would still be there by default but would show the deprecation warning
the most complete thing would be to have a version of 3.28 with and without the @ember/legacy-built-in-components addon

@ef4
Copy link
Contributor

ef4 commented Nov 14, 2022

Please run prettier so that this will get past the linters and we can see the full CI run.

@SergeAstapov
Copy link
Collaborator

@void-mAlex it won't hurt to add @ember/legacy-built-in-components for the ember-engines scenarios for all the versions, especially if it's simpler from tests setup perspective.

it's not ideal anyways, but IMO it's much more important to have ember-engines being included in CI than having extra @ember/legacy-built-in-components package for v3 cases.

@SergeAstapov
Copy link
Collaborator

@void-mAlex build fails as you haven't actually added @ember/legacy-built-in-components to dependencies

@void-mAlex
Copy link
Collaborator Author

please bear with me as running the test suites locally is very painful, so actually figuring out if I added the dependency correctly is hard (maybe just for me).
is there a way to tell the scenario runner to use a different TEMP directory on a drive that has more space?

@SergeAstapov I could also use some help with yarn
as far as I can tell I would also need to add the addon as a dev dependency for the @embroider/test-scenarios workspace
when I try
yarn workspace @embroider/test-scenarios add --dev @ember/legacy-built-in-components

I get this yarn error Invariant Violation: expected workspace package to exist for "eslint" Yarn version: 1.22.19

Node version:
16.16.0

Platform:
win32 x64

Trace:
Invariant Violation: expected workspace package to exist for "eslint"

as for the tests I can most of the time get a complete execution if I narrow it down to just that file npx qunit --require ts-node/register engines-test.ts

currently trying to see if I can get yarn to behave
think I tricked it by manually editing the tests/scenarios/package.json and then running a yarn install
seems like it's going further. been editing this comment for a while now for will update once I get even further

@void-mAlex
Copy link
Collaborator Author

pushed a set of changes that I believe should work, in order to get some feedback

locally I can't seem to get the tests to pass even though I've confirmed the following

{
  "name": "app-template",
  "version": "0.0.0",
  "private": true,
  "description": "Small description for app-template goes here",
  "repository": "",
  "license": "MIT",
  "author": "",
  "directories": {
    "doc": "doc",
    "test": "tests"
  },
  "scripts": {
    "build": "ember build",
    "build:production": "ember build -prod",
    "lint": "npm-run-all --aggregate-output --continue-on-error --parallel \"lint:!(fix)\"",
    "lint:fix": "npm-run-all --aggregate-output --continue-on-error --parallel lint:*:fix",
    "lint:hbs": "ember-template-lint .",
    "lint:hbs:fix": "ember-template-lint . --fix",
    "lint:js": "eslint . --cache",
    "lint:js:fix": "eslint . --fix",
    "start": "ember serve",
    "test": "ember test",
    "test:ember": "ember test"
  },
  "devDependencies": {
    "eager-engine": "0.0.0",
    "lazy-engine": "0.0.0",
    "@babel/core": "7.19.3",
    "@ember/optional-features": "2.0.0",
    "@ember/test-helpers": "2.8.1",
    "@embroider/compat": "1.9.0",
    "@embroider/core": "1.9.0",
    "@embroider/router": "1.9.0",
    "@embroider/test-setup": "1.8.3",
    "@embroider/webpack": "1.9.0",
    "@glimmer/component": "1.1.2",
    "@glimmer/tracking": "1.1.2",
    "babel-eslint": "10.1.0",
    "broccoli-asset-rev": "3.0.0",
    "ember-auto-import": "2.4.2",
    "ember-cli-app-version": "5.0.0",
    "ember-cli-babel": "7.26.11",
    "ember-cli-dependency-checker": "3.3.1",
    "ember-cli-htmlbars": "6.1.0",
    "ember-cli-inject-live-reload": "2.1.0",
    "ember-cli-sri": "2.1.1",
    "ember-cli-terser": "4.0.2",
    "ember-fetch": "8.1.1",
    "ember-load-initializers": "2.1.2",
    "ember-page-title": "7.0.0",
    "ember-qunit": "5.1.5",
    "ember-resolver": "8.0.3",
    "ember-template-lint": "4.12.0",
    "eslint": "7.32.0",
    "eslint-config-prettier": "8.5.0",
    "eslint-plugin-ember": "11.0.5",
    "eslint-plugin-node": "11.1.0",
    "eslint-plugin-prettier": "4.2.1",
    "eslint-plugin-qunit": "7.3.1",
    "loader.js": "4.7.0",
    "npm-run-all": "4.1.5",
    "prettier": "2.7.1",
    "qunit": "2.19.2",
    "qunit-dom": "2.0.0",
    "webpack": "5.74.0",
    "ember-source": "4.4.2",
    "ember-cli": "4.4.0",
    "ember-data": "4.4.1"
  },
  "engines": {
    "node": "14.* || >= 16"
  },
  "ember": {
    "edition": "octane"
  },
  "volta": {
    "node": "14.19.3",
    "yarn": "1.22.19"
  },
  "ember-addon": {
    "paths": [
      "lib/lazy-in-repo-engine"
    ]
  },
  "dependencies": {
    "ember-engines": "0.8.19",
    "ember-truth-helpers": "3.0.0",
    "@embroider/macros": "1.9.0",
    "@ember/legacy-built-in-components": "0.4.1"
  }
}

dependecy gets correctly added and installed to the generated temporary app (found in the $TMP folder); the above is the package json for the project

checked and the @ember/legacy-built-in-components folder exists inside node_modules

the error I see from the test run is

not ok 1 Chrome 107.0 - [undefined ms] - Global error: Uncaught Error: Could not find module `@ember/routing/link-component` imported
 from `(require)` at http://localhost:7357/assets/vendor.js, line 253

@void-mAlex
Copy link
Collaborator Author

think I just spotted the reason it's not working locally "ember-engines": "0.8.19",
and I believe I need at least "ember-engines": "0.8.22", for ember-engines/ember-engines#798

it's late now so I'll update it tomorrow and try again locally hopefully with more luck
can I assume it's safe to bump ember-engines up to the latest release 0.8.23? for the whole scenario?

@SergeAstapov
Copy link
Collaborator

@void-mAlex Looks there is a conflict, mind to refresh with latest main?

@void-mAlex
Copy link
Collaborator Author

tests now look green locally
conflict resolved as well

@ef4
Copy link
Contributor

ef4 commented Nov 15, 2022

please bear with me as running the test suites locally is very painful, so actually figuring out if I added the dependency correctly is hard (maybe just for me).
is there a way to tell the scenario runner to use a different TEMP directory on a drive that has more space?

Sorry, the CONTRIBUTING.md documentation is out of date here. What I would do for this one is:

  1. cd tests/scenarios
  2. yarn test:list to figure out the full name of the scenario you want. In this case, I'd probably start with release-engines-without-fastboot.
  3. yarn test:output --scenario release-engines-without-fastboot --outdir /path/to/wherever. This will create the scenario as a standalone project in /path/to/wherever.
  4. cd /path/to/wherever && yarn start to boot and debug the ember app.

@void-mAlex
Copy link
Collaborator Author

Sorry, the CONTRIBUTING.md documentation is out of date here.

#1283
to make the doc less out of date

@ef4 ef4 merged commit 191f6eb into embroider-build:main Nov 15, 2022
@ef4
Copy link
Contributor

ef4 commented Nov 15, 2022

Thanks, looks good. So I guess our engines support is actually pretty OK already!

Probably the next step in this area is for the folks who hit a problem with the link components to add that usage to these tests so we can see it break.

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.

3 participants