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

EFI: Using Node.js core testing #184

Open
sheplu opened this issue Feb 21, 2024 · 20 comments
Open

EFI: Using Node.js core testing #184

sheplu opened this issue Feb 21, 2024 · 20 comments

Comments

@sheplu
Copy link
Member

sheplu commented Feb 21, 2024

Motivation

Some testing libraries (and ecosystem) are a lot less maintained that a few years ago, and relying on them can be an issue or a maintenance burden in the coming years. Using the default test runner from Node.js core (part of Node.js 20) would give us a few advantages that ranges from better performances to lower security risks from third party libraries.
Some features may not be available now, so this discussion may be pushed back (or waiting to only keep version that support the Node.js test ecosystem).

Expectation

Slowly move and use the test runner in all libraries

Implementation

Update Node.js version
Use test runner ecosystem (test library, assertion, runner, coverage...)

Status

Part: Technical
Status:

Draft

Node.js now provides a complete (or almost) test ecosystem, including runners and reporters. Using that part of the stack to replace external dependencies for tests would lower the number of dependencies and reduce the maintenance

@dougwilson

This comment was marked as outdated.

@sheplu
Copy link
Member Author

sheplu commented Feb 21, 2024

The main issue is that the version below 18 are not supported and can be at risk, for example - if my memory is right, it is a previous openSSL version running and said version is now deprecated (and may be a security issue)

@dougwilson

This comment was marked as outdated.

@dougwilson

This comment was marked as outdated.

@UlisesGascon
Copy link
Member

I think that this initiative will be blocked until we have a solid plan for #194

@wesleytodd
Copy link
Member

wesleytodd commented Feb 22, 2024

I think we should rename the initiative which would cover both of these (and maybe a few more) to LTS Strategy. These are all related and I don't think we can discuss one without the other.

EDIT: I meant the stuff about version support, not if we used node test runner or not.

@ctcpip
Copy link
Member

ctcpip commented Feb 22, 2024

I like the node built-in test support, and embraced it, but in my attempts to adopt it very recently, I ran into some issues that indicated to me that its utility right now is limited to only very small/trivial uses. There is a lack of some basic features/support, and the design decision to throw AssertionError for any assert failure was one primary pain point. I spent time trying to wrap behavior, but it was making the code complex and more difficult to reason about, all just to get around its limitations and inherent design. So I abandoned that effort and added a light test lib instead.

@kibertoad
Copy link
Contributor

Ya, but Express.js supporting them doesn't mean users have to use them. I have to use Node.js low versions for various reasons and have very strong opinion on supporting old versions I need.

What is the oldest version that you are currently running in production?

The Express.js TC has always been aligned on only dropping Node.js support as strictly required, and for a test suite when the current one works perfectly fine seem a bit excessive

And it ended up causing quite a bit of stagnation in Express space, and scared away potential contributors. Supporting old versions is painful, and drastically limits what libraries and tools could be used.

Express 4 was used forever in the space and was good enough for the community. Are there any good reasons to artificially constrain the project, if those with the need to run legacy software on legacy systems will still always have Express 4 as a fallback? One may think about providing security updates to people who can't upgrade Node, but this point is not particularly valid. Old Node by itself is a huge security vulnerability, and it locks its users out of getting updates for the majority of libraries that they are likely to be using, so being locked out of newer Express versions is but a drop in the ocean all things considered.

@dougwilson
Copy link
Contributor

Hi @kibertoad I have left the Express.js project now due to personal reasons and my opinion above no longer carrier any weight from thr TC. It can be disregard really, unless someone else with the project states they share it. Thank you.

@ljharb
Copy link
Contributor

ljharb commented Feb 24, 2024

I think that "using node's test runner" should be a totally separate topic from "what node versions should be supported", and even if express only supported latest node, I would suggest not using the test runner. It's improving rapidly, but is organically designed rather than holistically/cohesively designed, and I'd suggest using something more self-consistent.

@wesleytodd
Copy link
Member

I think that "using node's test runner" should be a totally separate topic from "what node versions should be supported",

This ☝️ . If we think using node core's test runner is important it for sure means we need to decide on our LTS Strategy first. Can we move this conversation to #196?

unless someone else with the project states they share it.

I share it partly. Again, this is the wrong thread for that conversation so I will move the response over to #196.

@kibertoad
Copy link
Contributor

Can we restart the conversation now? As per our LTS strategy conclusions, if we see that setting our minimum to Node 18 gives tangible benefits for the project, we can do it. And being able to use core Node.js testing functionality is a pretty big deal.

The fact that it aligns us with the currently supported Node LTS versions is an accidental, but pleasant bonus.

@ljharb
Copy link
Contributor

ljharb commented Feb 29, 2024

Why exactly is it a "big deal"?

@kibertoad
Copy link
Contributor

@ljharb You've already expressed your preference for TAP in the past. Besides that, are there any other frameworks you are in favour of?

WebStorm, which is a very popular IDE, only supports Vitest, Jest, Mocha and Node core. Not everyone uses WebStorm, but it is very representative of what is considered mainstream and would be the most inviting to an average potential contributor. I am a big fan of vitest and strongly dislike jest, but either of the big four look like a sensible option to me.

@kibertoad
Copy link
Contributor

@ljharb It is a standard, native Node solution with good IDE support. Being part of the core, it is highly likely to be well-supported going forward. Generally if there is a native solution, I would say that a very strong reason is needed to prefer a custom library solution.
What benefits does TAP bring over it? Which parts of "organically grown" API do you dislike?

@ljharb
Copy link
Contributor

ljharb commented Feb 29, 2024

It's "experimental" which means it's not stable or standard yet, and that status should be a nonstarter for any serious project or organization.

I don't have a stake in express choosing TAP, altho it's certainly what I'd recommend - mocha or jest or vitest or whatever are fine. The important thing is that it is not in core so that its testing support is not inextricably shackled to node's own release schedule.

@kibertoad
Copy link
Contributor

kibertoad commented Feb 29, 2024

@ljharb Can you elaborate on why exactly you believe tap is better than Vitest or Mocha, simplicity aside? (which is a drawback as much as is an advantage, and hence very subjective).

Fair point on not tying to Node release schedule.

@ljharb
Copy link
Contributor

ljharb commented Feb 29, 2024

TAP output has lots of advantages, but basically all the test runners can produce that, so that's not really a pivot point. Particularly, it's that the code is "just javascript" - it doesn't need or use any build process or macros, you don't actually need a separate runner (you can exercise your tests with just node), it can easily run in a browser (which isn't relevant for express but may be for some of the other packages in the pile), and for tape specifically, it's that its extreme back compat support means that you're guaranteed to be able to test on whatever you want to test on.

I personally find tests written in tap/tape format far more readable and understandable than tests written in a BDD style. substack once gave an amazing talk on this at JSFest in 2014, but sadly i can't find any trace of it online.

@kibertoad
Copy link
Contributor

kibertoad commented Feb 29, 2024

Not needing a runner is not a particularly big advantage for IDE users.

I see, thank you for your perspective. My personal preferences are vastly different, but they are also personal, and highly subjective as such :)

Btw, modern Mocha is Node 14+ vs Vitest Node 18+, so if that is an important factor, we should take that into consideration. Although from DX perspective I find Vitest to be much superior.

@wesleytodd
Copy link
Member

FWIW, I think changing test frameworks in quite low on the list. Migrating them will be a HUGE lift, so we likely would need to call this a long term goal. I likely will not jump into this thread much because of the low priority imo, but dont let that stop anything.

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

No branches or pull requests

7 participants