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

Move to esbuild instead of browserify #1336

Merged
merged 14 commits into from
Dec 13, 2024
Merged

Conversation

relu91
Copy link
Member

@relu91 relu91 commented Dec 2, 2024

This PR should eventually completely move us from the old browserify build to a more modern toolchain based on esbuild and web/test-runner. There are still things to fix:

  • Rebase on main
  • Create a new workflow only for browser tests (we don't need the matrix for browser testing)
  • Configure web/test-runner to run on CI with the playwright or similar but use developer browser locally (?).
  • fix code style errors in web test runner

@relu91
Copy link
Member Author

relu91 commented Dec 4, 2024

I opted out browser tests by removing the test script in the browser bundle. So now the CI will only run node tests and in the next commit, I will create a workflow just for browser tests. The only incovient is that now developers should run npm run test:all instead of npm run test. test:all is automatically executed once you push too.

Copy link

codecov bot commented Dec 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.48%. Comparing base (38e6abb) to head (ce327a4).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1336      +/-   ##
==========================================
- Coverage   77.61%   77.48%   -0.14%     
==========================================
  Files          83       83              
  Lines       16576    16587      +11     
  Branches     1640     1638       -2     
==========================================
- Hits        12866    12852      -14     
- Misses       3652     3678      +26     
+ Partials       58       57       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@danielpeintner
Copy link
Member

The only incovient is that now developers should run npm run test:all instead of npm run test. test:all is automatically executed once you push too.

We don't mention the test commands at all (at the moment). Since the CI will run the check, developers will notice problems latest once pushing. Hence, I don't think we need to mention test:all (but we can of course).

@relu91 relu91 mentioned this pull request Dec 13, 2024
@relu91
Copy link
Member Author

relu91 commented Dec 13, 2024

Sorry there were complications with prettier (which I've updated to keep the new runner tests in check). I had to also re-run the format command since it seems that the new prettier has different rules. Sorry for making reviewing this harder... please let me know if there something wrong that you spot.

Regarding how the browser tests are run, they can run using npm run test:browser but only if the developer really want to test there. The new workflow should be activated once we merge this PR and should guarantee that even if the developer forgets we get always green light from browser too.

Copy link
Member

@danielpeintner danielpeintner left a comment

Choose a reason for hiding this comment

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

I didn't spot any obvious issue apart from "master" vs "main".

After fixing that, I am fine with merging...

Thanks a lot @relu91 !!

.github/workflows/browser-test.yml Outdated Show resolved Hide resolved
.github/workflows/browser-test.yml Outdated Show resolved Hide resolved
@relu91
Copy link
Member Author

relu91 commented Dec 13, 2024

I didn't spot any obvious issue apart from "master" vs "main".

After fixing that, I am fine with merging...

Thanks a lot @relu91 !!

You are right! that's why they didn't run!!! nice catch. pushed the fix.

@relu91
Copy link
Member Author

relu91 commented Dec 13, 2024

mmm it seems that we still require some python to be installed when doing development of node-wot. Let me try change the CI configuration.

@relu91
Copy link
Member Author

relu91 commented Dec 13, 2024

Now we hit a bug with optional dependencies:

Cannot find module @rollup/rollup-linux-x64-gnu. npm has a bug related to optional dependencies (npm/cli#4828).

This error is suggesting to re-generate the package-lock.json

Copy link
Member

@danielpeintner danielpeintner left a comment

Choose a reason for hiding this comment

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

I think now everything seems to work 👍

@relu91 relu91 merged commit 5ab4236 into eclipse-thingweb:master Dec 13, 2024
14 checks passed
@relu91 relu91 deleted the esbuild branch December 13, 2024 18:14
@relu91 relu91 mentioned this pull request Dec 13, 2024
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