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

chore: mark pino as flaky #1002

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

ruyadorno
Copy link
Member

Marking pino as flaking since it's failing in many platforms for both v18.x and v20.x release lines.

Refs: #988

Marking `pino` as flaking since it's failing in many platforms for both
`v18.x` and `v20.x` release lines.

Refs: nodejs#988
@codecov-commenter
Copy link

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (cd1b288) 96.44% compared to head (cc9f984) 93.17%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1002      +/-   ##
==========================================
- Coverage   96.44%   93.17%   -3.28%     
==========================================
  Files          28       28              
  Lines        2139     2139              
==========================================
- Hits         2063     1993      -70     
- Misses         76      146      +70     

see 7 files with indirect coverage changes

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

@mcollina
Copy link
Member

mcollina commented Oct 4, 2023

Should be fixed now.

@mcollina
Copy link
Member

mcollina commented Oct 4, 2023

here is a rerun of the last one I've seen with pino failing:

https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3304/

@mcollina
Copy link
Member

mcollina commented Oct 5, 2023

@ruyadorno my understanding is this is passing according to the current list of skipped environments: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3305/.

I don't expect it to pass on those other platforms, mostly because the errors are not in pino itself.

@RafaelGSS
Copy link
Member

I agree with @mcollina, it seems to be working as expected. New CITGM just for the sake of conscience https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3307/

@alfonsograziano
Copy link
Contributor

Pino is still failing on 2 platforms in the last run on v18.18

Any idea on what to do in this case?

@mcollina
Copy link
Member

mcollina commented Oct 8, 2023

Either lets skip it there or wait a few days that I can take a look.

@alfonsograziano
Copy link
Contributor

I'd say, since Pino is such an important tool in the ecosystem, is better to have a failing CI for a few days and wait for a fix, rather than skipping the module. Thanks a lot for the help Matteo, super appreciated 🙏

@mcollina
Copy link
Member

mcollina commented Oct 9, 2023

@alfonsograziano
Copy link
Contributor

Awesome Matteo, thanks for the quick fix 🚀
@ruyadorno I guess we can safely close this PR and also #988 now :)

@mcollina
Copy link
Member

I think this is fixed. Let me know if there is anything else here.

@ruyadorno
Copy link
Member Author

Well it doesn't look like it has been completely fixed as it's looking like it's still timing out on both rhel8-ppc64le and win-vs2019 even in that fresh run (#3311). We should keep an eye on future runs to see if it's just these platforms that can be marked flaky and then update this PR accordingly.

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.

None yet

6 participants