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

test(nestjs-core): fixed nestjs-core tav to work on all node versions #1148

Merged

Conversation

osherv
Copy link
Member

@osherv osherv commented Sep 1, 2022

Which problem is this PR solving?

test-all-versions for node8 failed on nestjs-core@8 tests.
Here's the error:
https://github.com/open-telemetry/opentelemetry-js-contrib/runs/8134447972?check_suite_focus=true

Pinning the tests to version 8.0.0 fix the problem, and now it works for all node versions.

  • Ran npm run test-all-versions for the edited package(s) on the latest commit if applicable.

@osherv osherv requested a review from a team September 1, 2022 18:00
@osherv osherv changed the title fix(nestjs-core): fixed tav to work on all node versions fix(nestjs-core): fixed nestjs-core tav to work on all node versions Sep 1, 2022
@codecov
Copy link

codecov bot commented Sep 1, 2022

Codecov Report

Merging #1148 (9b4994d) into main (59f7bce) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1148   +/-   ##
=======================================
  Coverage   96.07%   96.07%           
=======================================
  Files          14       14           
  Lines         892      892           
  Branches      191      191           
=======================================
  Hits          857      857           
  Misses         35       35           

@dyladan
Copy link
Member

dyladan commented Sep 1, 2022

We're not getting the latest versions of these dependencies in our tests now though right? Isn't it better to test in an environment as similar to what a user would see as possible? It might be better to drop node 8 testing

/cc @open-telemetry/javascript-maintainers

@osherv
Copy link
Member Author

osherv commented Sep 2, 2022

@dyladan yes you're right. Maybe it's better to remove the whole node8 support.
Let me know if that's preferred :)

@dyladan
Copy link
Member

dyladan commented Sep 2, 2022

Waiting to hear what other maintainers think but removing node 8 is probably the way to go

@osherv
Copy link
Member Author

osherv commented Sep 7, 2022

@dyladan @rauno56 According to this pr: #1065
I think that you agree that the best this is to remove the support for tav in node8.

@blumamir
Copy link
Member

blumamir commented Sep 7, 2022

Thank you @osherv for fixing this.

I support dropping node8 support for nest-js instrumentation, but it seems that nestjs (at least with latest version 9) suuports node >= 12.9.0. So it should fail on node10 as well, right? any idea why we see problem for node8 only?

@blumamir blumamir changed the title fix(nestjs-core): fixed nestjs-core tav to work on all node versions test(nestjs-core): fixed nestjs-core tav to work on all node versions Sep 7, 2022
@osherv
Copy link
Member Author

osherv commented Sep 7, 2022

@blumamir Thanks :)
I have no idea, i can dig into this, but we can drop this version as well XD

@rauno56
Copy link
Member

rauno56 commented Sep 13, 2022

@osherv, I agree - let's add skip for 10 as well and merge this.

@osherv
Copy link
Member Author

osherv commented Sep 13, 2022

@rauno56 Thanks, added 10 version

@rauno56 rauno56 merged commit c1a8622 into open-telemetry:main Sep 14, 2022
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.

4 participants