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

lookup: re-add fastify #973

Merged
merged 5 commits into from
Oct 22, 2023
Merged

lookup: re-add fastify #973

merged 5 commits into from
Oct 22, 2023

Conversation

mcollina
Copy link
Member

No description provided.

@mcollina
Copy link
Member Author

@Trott how can I test this in CITGM smoker?

@Trott
Copy link
Member

Trott commented Sep 11, 2023

@Trott how can I test this in CITGM smoker?

EDIT: It looks like the below stuff didn't work so I guess I didn't quite get it right. The GIT_REMOTE_REF probably refers to the nodejs/node repository and not the nodejs/citgm repository.....

If I didn't make any mistakes, it's running here: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3227/

Here are the steps I took to do it:

  1. Go to https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/.
  2. Select "Build With Parameters" on the left.
  3. Enter refs/pull/973/head for the GIT_REMOTE_REF
  4. Enter citgm fastify for the CITGM_COMMAND (so that it only runs a test for fastify and not every module in lookup.json)
  5. Use the Build button

@Trott
Copy link
Member

Trott commented Sep 11, 2023

@Trott how can I test this in CITGM smoker?

EDIT: It looks like the below stuff didn't work so I guess I didn't quite get it right. The GIT_REMOTE_REF probably refers to the nodejs/node repository and not the nodejs/citgm repository....

Maybe someone on @nodejs/citgm knows the right thing to do to test this PR?

lib/lookup.json Outdated Show resolved Hide resolved
@richardlau
Copy link
Member

@Trott how can I test this in CITGM smoker?

EDIT: It looks like the below stuff didn't work so I guess I didn't quite get it right. The GIT_REMOTE_REF probably refers to the nodejs/node repository and not the nodejs/citgm repository....

Maybe someone on @nodejs/citgm knows the right thing to do to test this PR?

If you want to use the Jenkins job, use the CITGM parameter:
image
e.g. for this PR, try nodejs/citgm#fastify-fix.

@targos
Copy link
Member

targos commented Sep 12, 2023

@mcollina
Copy link
Member Author

Let's see what the smoker says: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3230/.

@mcollina
Copy link
Member Author

@codecov-commenter
Copy link

codecov-commenter commented Sep 24, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (b8193a7) 96.44% compared to head (9db7bc9) 96.44%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #973   +/-   ##
=======================================
  Coverage   96.44%   96.44%           
=======================================
  Files          28       28           
  Lines        2139     2139           
=======================================
  Hits         2063     2063           
  Misses         76       76           

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

@mcollina
Copy link
Member Author

lib/lookup.json Outdated
"fastify": {
"maintainers": ["mcollina"],
"prefix": "v",
"skip": ["s390x", "aix", "aix72-ppc64"],
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"skip": ["s390x", "aix", "aix72-ppc64"],
"skip": ["s390x", "aix"],

You don't need aix72-ppc64 if aix is also in this set. Skips are only considered for citgm-all -- if you're running citgm <module> CITGM assumes you wanted to run for that module.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah awesome, so I don't need a full green run.

@simone-sanfratello
Copy link

simone-sanfratello commented Oct 5, 2023

Hello, I'm investigating the failing tests, tackling one by one.

For example this one

https://github.com/fastify/fastify/blob/main/test/custom-http-server.test.js#L11

Looks like it gets timeout on dns lookup

const localAddresses = await dns.lookup('localhost', { all: true })

I'm adding more logs to get information, btw, what the dns lookup is suppose to return in those machines?

@mcollina
Copy link
Member Author

mcollina commented Oct 9, 2023

@mcollina
Copy link
Member Author

@mcollina
Copy link
Member Author

@mcollina mcollina marked this pull request as ready for review October 19, 2023 10:59
@mcollina mcollina changed the title Trying to fix Fastify CITGM lookup: re-add fastify Oct 19, 2023
@mcollina
Copy link
Member Author

@targos can you confirm everything is passing correctly? I think we got there eventually.

@targos
Copy link
Member

targos commented Oct 19, 2023

LGTM

@mcollina mcollina merged commit 876adc0 into main Oct 22, 2023
8 checks passed
@mcollina mcollina deleted the fastify-fix branch October 22, 2023 09:28
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.

7 participants