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.

Sign up for GitHub

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

Jump to bottom

Add engine-specific 'start' commands for running integration tests/examples #95

Merged
merged 4 commits into from
Mar 28, 2022
Merged

Add engine-specific 'start' commands for running integration tests/examples #95

merged 4 commits into from
Mar 28, 2022

Conversation

prestomation
Copy link
Contributor

Issue #, if available:

Description of changes:

In the root, you may now run start-core, start-three, or start-babylon.

These all run webpack dev server, but will open relevant tabs for the package you have selected. This is either integration tests, or both integration tests and examples.

More work is still required to integrate the integration test code/pages into webpack so they support hot reload

This change also enables source maps for non-production builds.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@prestomation
Add engine-specific 'start' commands for running integration tests/ex…
3624b32 
…amples.

In the root, you may now run `start-core`, `start-three`, or `start-babylon`.

These all run webpack dev server, but will open relevant tabs for the package you have selected. This is either integration tests, or both integration tests and examples.

More work is still required to integrate the integration test code/pages into webpack so they support hot reload

This change also enables source maps for non-production builds.
runpiw
runpiw approved these changes Mar 22, 2022
View reviewed changes
README.md Outdated
- [Babylon.js](packages/amazon-sumerian-hosts-babylon/test/integration_test/README.md)
- [Three.js](packages/amazon-sumerian-hosts-three/test/integration_test/README.md)

### Examples Folder
Each of the implementation packages have their own examples attached to them. Each of these files contain enough code to stand up and verify the code is working properly. To verify, open up a local server for the package you are modifying:
```
npm run --workspaces=./packages/amazon-sumerian-hosts-{package} start
npm run start-{engine-name} start
Copy link
Contributor

Choose a reason for hiding this comment

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

i think the last start is not necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, thanks!

Krxtopher
Krxtopher suggested changes Mar 23, 2022
View reviewed changes
Copy link
Contributor

@Krxtopher Krxtopher left a comment

Choose a reason for hiding this comment

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

Something in this PR has broken the Babylon demos. When attempting to access any of the demos in the browser the JS console reports a 404 for the main JS file belonging to the demo (ex. "helloWorldDemo.js"). Perhaps this has something to do with the removal of the copy-webpack-plugin?

@Krxtopher
Copy link
Contributor

Forgot to mention this in the review I just submitted, but another problem with this PR is that the code scan is reporting a number of CodeQL issues.

@prestomation
Copy link
Contributor Author

Some more changes:

  1. I've moved demo-credentials.js to the root of the repo. It is onerous for devs to insert what is likely the same cognito pool many places. I've created a fake API in webpack at /devConfig.json where webpack serves up the cognito pool and it is fetched via the samples. Ultimately I do thing we should serve up everything via webpack and probably HtmlWebpackPlugin

  2. I tried to change this to a module with type: module in package.json. If you are reading this, do not attempt this until Karma 7! It doesn't seem like it can work without some really hacky stuff and I could not get it to work even then: Support for ESM config files (with "type": "module") karma-runner/karma#3677. My workaround is to change demo-credentials.js to be node-style instead of a ESM.

  3. A release build won't build any of the examples. These don't work without webpack server because of file location and our devConfig endpoint, so I think it is confusing/misleading to add these to the build output. They wouldn't work anyway without cognito so I don't think this is so bad, but we need to figure out how to test these in CI and how to pull a sample cognito Id from github secrets/etc

  4. Regarding CodeQL: these were from the test files. I'm not sure why, as they are not in the repo and only in the build output(in an earlier rev) codeql wouldn't tell me the problem. Now that they aren't in the build output CodeQL doesn't see these errors anymore..

@prestomation
Copy link
Contributor Author

I've also tested all of the examples and integration tests and have fixed these

@prestomation prestomation merged commit 4662df5 into aws-samples:mainline2.0 Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Krxtopher Krxtopher Krxtopher approved these changes

@runpiw runpiw runpiw approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@prestomation @Krxtopher @runpiw