Skip to content

Miscellaneous repo cleanup #560

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

Merged
merged 15 commits into from
Oct 13, 2021
Merged

Miscellaneous repo cleanup #560

merged 15 commits into from
Oct 13, 2021

Conversation

aomarks
Copy link
Member

@aomarks aomarks commented Oct 13, 2021

  • Move tests into new lit-dev-tests package because:
    • Seems a bit more logical. Everything else lives in a package.
    • It allows us to skip installing test-related dependencies on Docker build.
  • Re-organize GitHub action workflows. Most stuff was crammed into the one build action before.
  • Run playground-plugin unit tests in CI (under new test:unit grouping).
  • Combines golden screenshot generation + artifact upload into the main integration-tests action, but only run it if the tests fail.
  • Use playwright's built-in webServer feature instead of run-p to launch the server used for integration testing.
  • Simplify Prettier setup and check formatting in CI.
  • Rename lit-dev-tools to lit-dev-tools-cjs to distinguish it from lit-dev-tools-esm.
  • Only run the manrope font download step if it hasn't already run (it was needlessly running every time we installed anything or bootstrapped through lerna)
  • Factor out a common tsconfig.base.json.
  • Update dependencies.

@github-actions
Copy link

github-actions bot commented Oct 13, 2021

A live preview of this PR will be available at the URL(s) below.
The latest URL will be appended to this comment on each push.
Each build takes ~5-10 minutes, and will 404 until finished.

https://pr560-d136940---lit-dev-5ftespv5na-uc.a.run.app/
https://pr560-5ee8310---lit-dev-5ftespv5na-uc.a.run.app/
https://pr560-f2e924b---lit-dev-5ftespv5na-uc.a.run.app/
https://pr560-c331df8---lit-dev-5ftespv5na-uc.a.run.app/

@aomarks aomarks force-pushed the integration-tests-wip branch 4 times, most recently from a66584d to d136940 Compare October 13, 2021 14:22
Copy link
Contributor

@AndrewJakubowicz AndrewJakubowicz left a comment

Choose a reason for hiding this comment

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

Checked in the code and tried some test, build and format commands. Also skimmed some of the preview url. Not 100% sure where the risky areas are but from some random feature checking everything is working.

This cleanup is fantastic. Great formatting and factoring. Also thank you for attaching those search tests to the CI.

A little unsure why one of the CI checks is stuck on the build.
Apart from that this is a huge improvement! LGTM!

Comment on lines +24 to +27
- if: ${{ always() && steps.test.outcome == 'failure' }}
run: npm run test:integration:update-golden-screenshots

- if: ${{ always() && steps.test.outcome == 'failure' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Brilliant!

"test:integration:install-deps": "npx playwright install-deps chromium",
"test:integration:run": "npx playwright test",
"test:integration:update-golden-screenshots": "npx playwright test --update-snapshots"
"test:unit": "lerna run test --scope lit-dev-tools-cjs --scope lit-dev-tools-esm",
Copy link
Contributor

Choose a reason for hiding this comment

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

This was a bug - sorry! Good find and fix! Thank you!

Base automatically changed from share-button-2 to main October 13, 2021 20:32
@aomarks aomarks force-pushed the integration-tests-wip branch from 5ee8310 to f2e924b Compare October 13, 2021 20:49
@aomarks aomarks merged commit f701640 into main Oct 13, 2021
@aomarks aomarks deleted the integration-tests-wip branch October 13, 2021 21:09
aomarks added a commit that referenced this pull request Nov 2, 2021
Fixes a regression from #560 where we accidentally dropped from "target": "esnext" to "target": "es2020" in the tsconfig.json for sample files (I made a common tsconfig.base.json which defaults to es2020 but forgot to override that in the samples one). We want esnext output for samples because we want to emit standard static class fields (note that @lit/ts-transformers switches instance class field initializers to be constructor-initialized if they are decorated with @Property, to avoid that incompatibility).
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