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

esm: mordernize old tests #54354

Closed
wants to merge 12 commits into from

Conversation

puskin94
Copy link
Contributor

Doing a round of fast cleanup of old open and stalled PRs.
Making sure this PR is moving along addressing the comments in the original discussion.
I added the author of the original PR in the commit message.

Co-Authored-By: Jacob Smith

@nodejs-github-bot nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Aug 13, 2024
@puskin94 puskin94 requested a review from simoneb August 13, 2024 21:00
Copy link
Contributor

@simoneb simoneb left a comment

Choose a reason for hiding this comment

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

LGTM

@aduh95 aduh95 force-pushed the test-modernize-old-esm-tests branch from 03c10ae to a0130aa Compare August 14, 2024 08:16
Co-Authored-By: Jacob Smith <[email protected]>
@aduh95 aduh95 force-pushed the test-modernize-old-esm-tests branch from a0130aa to b0f3cc2 Compare August 14, 2024 08:35
Copy link

codecov bot commented Aug 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.09%. Comparing base (02b3095) to head (b0f3cc2).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #54354      +/-   ##
==========================================
- Coverage   87.09%   87.09%   -0.01%     
==========================================
  Files         648      648              
  Lines      182216   182216              
  Branches    34969    34963       -6     
==========================================
- Hits       158703   158692      -11     
- Misses      16789    16796       +7     
- Partials     6724     6728       +4     

see 19 files with indirect coverage changes

@aduh95
Copy link
Contributor

aduh95 commented Aug 14, 2024

I've force pushed to fix the Co-Authored-By so it can be counted by our tooling as a contribution from Jacob. Note that it seems GitHub is not able to link the commit to your GitHub account – which is not at all a problem to get this landed, but maybe something you'd like to fix so the commit can be correctly credited to you.

@targos
Copy link
Member

targos commented Aug 14, 2024

Note there's a typo in the commit message.

@puskin94
Copy link
Contributor Author

Yeah Jacob's email looks wrong in your commit. Any hint on how I should fix the commit message? Should I just amend? If so, which email for Jacob should I use?

When looking around I only found a "redacted" one from github

@aduh95
Copy link
Contributor

aduh95 commented Aug 14, 2024

Jacob's email is fine as is. It is not associated with his GitHub account but I believe that's on purpose. Our tooling does not use GitHub info to count for participation so that's not a big deal.

@puskin94 if you want to amend the commit message to fix the typo, that'd be great. At the same time, consider either changing the commit author email address to one linked with your GitHub account (currently it is giovanni dot bucci at fleetster dot net), or add the current email address to your GitHub account, or leave things as is if you're not interested in having this commit linked with your GitHub account.

puskin94 and others added 8 commits August 14, 2024 13:15
Change posix.join to use array.join instead of additional assignment.

PR-URL: nodejs#54331
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Co-Authored-By: Jacob Smith <[email protected]>
Co-Authored-By: Jacob Smith <[email protected]>
Co-Authored-By: Jacob Smith <[email protected]>
@puskin94
Copy link
Contributor Author

@aduh95 I created some mess with the rebase, I wasn't able to do it because somehow the interactive one is not showing me commits happened after 60518269bc37ea78719dc50a4381d9a106a2b734 .
Having spent already too much time on this, I prefer closing this one and create a new PR from a clean slate :)

@puskin94 puskin94 closed this Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants