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

Fix duplicated test descriptions #54140

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

unseen1980
Copy link

Some unit tests had duplicate names. Please review these small changes.

Thanks
Christos

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jul 31, 2024
Copy link

codecov bot commented Jul 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.61%. Comparing base (b4fd1fd) to head (b55ca4c).
Report is 572 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #54140      +/-   ##
==========================================
+ Coverage   87.06%   87.61%   +0.54%     
==========================================
  Files         643      650       +7     
  Lines      181576   182834    +1258     
  Branches    34894    35383     +489     
==========================================
+ Hits       158088   160183    +2095     
+ Misses      16759    15924     -835     
+ Partials     6729     6727       -2     

see 183 files with indirect coverage changes

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@RedYetiDev RedYetiDev added test Issues and PRs related to the tests. strip-types Issues or PRs related to strip-types support labels Aug 10, 2024
@RedYetiDev
Copy link
Member

RedYetiDev commented Aug 10, 2024

Hey @unseen1980. Just want to let you know the first commit is authored by @ckoutsiaris (just making sure this is intentional). Additionally, could you remove the trailing .? Punctuation isn't supposed to be in commit messages.

Thank you!

@unseen1980 unseen1980 changed the title Fix duplicated test descriptions. Fix duplicated test descriptions Aug 11, 2024
@unseen1980
Copy link
Author

Hey @unseen1980. Just want to let you know the first commit is authored by @ckoutsiaris (just making sure this is intentional). Additionally, could you remove the trailing .? Punctuation isn't supposed to be in commit messages.

Thank you!

Hi @RedYetiDev yes that is fine, same person :) I removed the . and I reset the commit history, please review again. Thank you 👍

@RedYetiDev
Copy link
Member

Sorry I forgot to mention this earlier, but in order to land, the commit needs to be prefixed with a subsystem, followed by an active verb.

For example, test: remove ...

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 12, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 12, 2024
@nodejs-github-bot
Copy link
Collaborator

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 14, 2024
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 14, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Aug 20, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 20, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@jakecastelli jakecastelli added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Aug 24, 2024
@nodejs-github-bot
Copy link
Collaborator

@marco-ippolito marco-ippolito added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 28, 2024
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 28, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/54140
✔  Done loading data for nodejs/node/pull/54140
----------------------------------- PR info ------------------------------------
Title      Fix duplicated test descriptions (#54140)
Author     Christos Koutsiaris <koutsiaris@gmail.com> (@unseen1980, first-time contributor)
Branch     unseen1980:duplicated-test-names -> nodejs:main
Labels     test, author ready, needs-ci, commit-queue-squash, strip-types
Commits    2
 - test: remove duplicated test descriptions
 - fixup! test: remove duplicated test descriptions
Committers 1
 - Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/54140
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54140
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Wed, 31 Jul 2024 11:00:48 GMT
   ✔  Approvals: 5
   ✔  - Michael Dawson (@mhdawson) (TSC): https://github.com/nodejs/node/pull/54140#pullrequestreview-2212768019
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/54140#pullrequestreview-2231624173
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/54140#pullrequestreview-2238513235
   ✔  - Jake Yuesong Li (@jakecastelli): https://github.com/nodejs/node/pull/54140#pullrequestreview-2258732846
   ✔  - Marco Ippolito (@marco-ippolito) (TSC): https://github.com/nodejs/node/pull/54140#pullrequestreview-2265320137
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-08-27T14:45:02Z: https://ci.nodejs.org/job/node-test-pull-request/61551/
- Querying data for job/node-test-pull-request/61551/
   ✔  Last Jenkins CI successful
   ⚠  PR author is a new contributor: @unseen1980(koutsiaris@gmail.com)
   ⚠  - commit 4e93a87d4fef is authored by christos.koutsiaris@sap.com
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/10591774472

@marco-ippolito
Copy link
Member

I think the issue is that the commit email does not match the account email:
commit 4e93a87d4fef is authored by christos.koutsiaris@sap.com
@unseen1980

@unseen1980
Copy link
Author

I think the issue is that the commit email does not match the account email: commit 4e93a87d4fef is authored by christos.koutsiaris@sap.com @unseen1980

Apologies for the mess, I just tried to amend the author of that commit. Not sure if it worked.

@marco-ippolito
Copy link
Member

marco-ippolito commented Aug 31, 2024

Can you squash commits into one and remove merge commits?

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 1, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 1, 2024
@nodejs-github-bot

This comment was marked as outdated.

@unseen1980
Copy link
Author

Can you squash commits into one and remove merge commits?

Hi @marco-ippolito it should be ok now, apologies for the messup. Thank you 👍

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@RedYetiDev RedYetiDev removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 10, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@unseen1980
Copy link
Author

Hello, is there anything I need to do from my side?

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. strip-types Issues or PRs related to strip-types support test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants