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

chore(ci): wire test/ to ci #3263

Merged
merged 4 commits into from
Mar 7, 2022

Conversation

rwaskiewicz
Copy link
Contributor

@rwaskiewicz rwaskiewicz commented Mar 3, 2022

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally and any changes were pushed
  • Unit tests (npm test) were run locally and passed (npm test.prod was run)
  • E2E Tests (npm run test.karma.prod) were run locally and passed
  • Prettier (npm run prettier) was run locally and passed

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Projects under the test/ directory aren't type checked, which led to requiring a quick
fix of #3259 to unblock Monday's release. We should
have caught this in #3211, but CI wasn't hooked up here

GitHub Issue Number: N/A

What is the new behavior?

this commit adds building several applications in the test/ directory to
the project's root level test.prod npm script. while wiring these up,
it was determined there are two projects in test/ that do not compile.
fixing these at a high level have been included in two separate commits in this
PR.

Does this introduce a breaking change?

  • Yes
  • No

Testing

I've broken CI intentionally (and unintentionally 😆) a few times getting the todo and ionic apps to build, as well
as verifying that a broken browser-compile app will also fail CI

"analysis": "node ./.scripts/analysis.js",
"build": "npm run build.hello-world && npm run build.hello-vdom && npm run build.todo && npm run build.end-to-end && npm run build.ionic && npm run analysis",
"TODO-STENCIL-389": "echo Remove the exit call below",
"analysis": "exit 0 && node ./.scripts/analysis.js",
Copy link
Contributor Author

@rwaskiewicz rwaskiewicz Mar 3, 2022

Choose a reason for hiding this comment

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

This command can have exit 0 here as it's run in a subshell, and therefore don't cause premature ending of the parent script.

This script hasn't been run since Stencil v1.9, which is why I want to take the time to really understand
what it's doing (it writes the README file in this test dir) so that we can understand changes in file size & count

this commit adds building most projects in the `test/` dir to stencil's
ci system. the `test/package.json` is updated to include the
`browser-compile` subdir, as well as a few script renames. those script
names are propagated up to the project's root level `package.json` so
that they may be called one of two ways:

1. in ci via `test.analysis`
2. locally as a part of the 'test all' script, `test.prod`
@rwaskiewicz rwaskiewicz force-pushed the rwaskiewicz/add-ci-checks-browser-compile branch from fbe16af to 97fe0a8 Compare March 4, 2022 14:01
- add a `package-lock.json` to allow for `npm ci` installs in ci
- remove `exit 0` call in `test/package.json` to allow for running build
- add a `package-lock.json` to allow for `npm ci` runs in ci
  - this required updating the ionic app's `.npmrc` and `.gitignore`
- update the app to use ionic v6
- remove `exit 0` call from `package.json` in test dir
@rwaskiewicz rwaskiewicz changed the title Rwaskiewicz/add ci checks browser compile chore(ci): wire test/ to ci Mar 4, 2022
@rwaskiewicz rwaskiewicz marked this pull request as ready for review March 4, 2022 14:47
@rwaskiewicz rwaskiewicz requested a review from a team March 4, 2022 14:48
package-lock=true
Copy link
Contributor

Choose a reason for hiding this comment

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

Eventually we should just delete the .npmrcs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed - for posterity (aka future me), the reason we kept them was that there is a test/.npmrc file that has package-lock=false in it. To keep the number of changes minimal, we overrode the value in the subdirectory to avoid side effects on other projects

@rwaskiewicz rwaskiewicz merged commit 6f99212 into main Mar 7, 2022
@rwaskiewicz rwaskiewicz deleted the rwaskiewicz/add-ci-checks-browser-compile branch March 7, 2022 18:36
rwaskiewicz added a commit that referenced this pull request Jan 24, 2023
this commit updates the typings for the analysis task. the team
re-enabled these tasks/tests (for the most part) in #3263.
at the same time, a commit that updates typings for onInput
events was sitting in the v3.0.0-dev branch,
#3135

upon merging main into v3.0.0-dev, the build broke. this commit fixes
the typings to align with the latter commit.
rwaskiewicz added a commit that referenced this pull request Jan 25, 2023
this commit updates the typings for the analysis task. the team
re-enabled these tasks/tests (for the most part) in #3263.
at the same time, a commit that updates typings for onInput
events was sitting in the v3.0.0-dev branch,
#3135

upon merging main into v3.0.0-dev, the build broke. this commit fixes
the typings to align with the latter commit.
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.

2 participants