node
node copied to clipboard
nodejs
esm: modernize old tests
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
Sorry I forgot to ask you that earlier, but the subsystem is actually wrong since it only touches the test/ folder. It should be test: refactor test-esm-type-field-errors or something like that.
The instructions on how to do it:
# first make sure you are on top of the right commit
git fetch https://github.com/nodejs/node.git bade48fc9662dd0a63f88a8a12d0c824f435276d
git reset FETCH_HEAD --hard
# amend the commit message
git commit --amend
# Force push to your branch
git push origin HEAD:modernize-old-esm-tests --force-with-lease
Let me know if you prefer I do that for you.
You might want to reopen the other PR btw, so the approval and the wait time do not reset. Let me know if you want me to push the correct commit over there instead. But it's also fine to use a new PR if that's what you prefer.
@aduh95 no worries!
The process of amending a commit is (kinda) clear in my head, it is not the first time I do it!
Somehow in the previous PR the git history was completely fine everywhere locally but not in the "interactive amend" file, it was just showing me 8 commits over the 12 that were pushed. I also asked around and nobody was able to come up with a decent solution, so I decided to create a new PR instead of wasting my time :)
No worries, this MR is nor critical, urgent or important, it can wait 1-2 days :)
Thanks for your support btw!
No issues, it's probably my bad, I supsect my force-push to your branch might have caused the issues you were seeing. FWIW git reset FETCH_HEAD --hard is often the solution :)
Sorry for the bike shedding, but test: modernize old tests feels too broad for the diff, because we're only updating one test file. IMO test: refactor `test-esm-type-field-errors` would be a better commit title.
@aduh95 no worries! Changed the commit message, again :)
and about git reset FETCH_HEAD --hard: I avoid it as much as I can, so much that I prefer to create a new PR 😄
Apologies again, I realize only now there's indeed a copy paste issue with Jacob's email:
https://github.com/nodejs/node/blob/a3ff3e8cd47a0393b5e4b123b12be334a757e091/README.md#L356
I feel bad asking you again for a force push, but hopefully this time it's the last one: can you please add the missing e to Jacob's email? Or let me know if you prefer I do that for you.
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 87.09%. Comparing base (
6051826) to head (f3a94cd). Report is 215 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@
## main #54368 +/- ##
==========================================
- Coverage 87.10% 87.09% -0.02%
==========================================
Files 648 648
Lines 182216 182217 +1
Branches 34966 34955 -11
==========================================
- Hits 158720 158694 -26
- Misses 16787 16818 +31
+ Partials 6709 6705 -4
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}