Enable testing with vitest #13

glen · 2024-10-25T17:05:36Z

glen commented

2024-10-25 17:05:36 +00:00

Resolves #12.

glen added 2 commits 2024-10-25 17:05:37 +00:00

test: Enable testing with vitest ef545b014d

chore: Remove code from index now redundant with tests 9b862b78b5

glen requested review from josdejong 2024-10-25 17:05:43 +00:00

glen commented

2024-10-25 17:09:32 +00:00

@josdejong This works for me, but I am not at all confident that it will run on Windows, as it basically needs to re-do the module resolution of the @/core/Dispatcher style of imports all over again for testing, and I am very concerned that the code I put in etc/vitest.config.ts is not portable to Windows. Would you give this a try (don't forget to pnpm install) and let me know if it works or take a stab at fixing the code if it doesn't? I suspect I need to use some node 'path' module functions for combining the path and file names there, rather than direct string manipulation like I am doing. Thanks so much!

@josdejong This works for me, but I am not at all confident that it will run on Windows, as it basically needs to re-do the module resolution of the `@/core/Dispatcher` style of imports all over again for testing, and I am very concerned that the code I put in `etc/vitest.config.ts` is not portable to Windows. Would you give this a try (don't forget to `pnpm install`) and let me know if it works or take a stab at fixing the code if it doesn't? I suspect I need to use some node 'path' module functions for combining the path and file names there, rather than direct string manipulation like I am doing. Thanks so much!

josdejong commented

2024-10-28 09:30:48 +00:00

All runs fine on Windows, nice job :)

Two thoughts:

I'm used to putting tests in a separate file with a name like .test.ts, and without the if (import.meta.vitest) conditionals, since you can let vitest run all .test.ts files (it does so by default). For example a file Dispatcher.ts and then alongside it a file Dispatcher.test.ts which tests all of the functionality of the dispatcher. Does that make sense?
Since we have our separate build step to generate types, we cannot easily run a test for a small part of the application. If running tests starts taking too much time (in the future), we should figure out a way to run a small part of it in a fast way. What makes vitest so great is that it can run in watch mode whilst you're developing, and when you change a single source file, it will automatically re-run the tests associated with that.

All runs fine on Windows, nice job :) Two thoughts: 1. I'm used to putting tests in a separate file with a name like `.test.ts`, and without the `if (import.meta.vitest)` conditionals, since you can let vitest run all `.test.ts` files (it does so by default). For example a file `Dispatcher.ts` and then alongside it a file `Dispatcher.test.ts` which tests all of the functionality of the dispatcher. Does that make sense? 2. Since we have our separate build step to generate types, we cannot easily run a test for a small part of the application. If running tests starts taking too much time (in the future), we should figure out a way to run a small part of it in a fast way. What makes vitest so great is that it can run in watch mode whilst you're developing, and when you change a single source file, it will automatically re-run the tests associated with that.

josdejong added 1 commit 2024-10-28 09:47:36 +00:00

fix: do not run vitest in watch mode (since we rely on a build step right now) 11a0e22f96

josdejong commented

2024-10-28 09:48:28 +00:00

I've pushed a commit to not run vitest in watch mode for now. I think later on we can look into running the build, reflect, and test scripts all in a watch mode. I don't think that is the highest priority right now though.

glen commented

2024-10-28 22:34:04 +00:00

OK, glad this works for you and I agree watch mode is not useful at the moment since it doesn't know to re-do our extra build step.

On the location of the tests: I had not used vitest before and saw that it allows tests alongside code. That seemed very sensible to me: like with documentation that gets the most attention and care and is most likely to be maintained correctly when it is right next to the code that is being documented (the main motivation for documentation extraction from source files, as is done at least to some extent in mathjs), it seemed to me that exactly the same arguments support putting tests right in the source files that implement whatever is being tested. Plus there's an added benefit in the case of testing: all of the top-level symbols in that source file are still in scope in the tests, so you don't have to worry about importing them. Finally, I think this tests-in-source-code is actually a common paradigm in languages in which documentation comments can have examples sections; there is typically a tool that runs such examples as unit tests.

So since this is a prototype anyway, how about we try the in-source-file tests and see how they feel as the project develops?

Of course, if you have a strong preference that the tests be separated into different files from the implementing code itself, I won't block a PR that moves the tests in that way and sets up a convention for where the tests should be located (to me that was a final small motivation for in-source tests; I didn't have to come up with any such convention).

OK, glad this works for you and I agree watch mode is not useful at the moment since it doesn't know to re-do our extra build step. On the location of the tests: I had not used vitest before and saw that it allows tests alongside code. That seemed very sensible to me: like with documentation that gets the most attention and care and is most likely to be maintained correctly when it is right next to the code that is being documented (the main motivation for documentation extraction from source files, as is done at least to some extent in mathjs), it seemed to me that exactly the same arguments support putting tests right in the source files that implement whatever is being tested. Plus there's an added benefit in the case of testing: all of the top-level symbols in that source file are still in scope in the tests, so you don't have to worry about importing them. Finally, I think this tests-in-source-code is actually a common paradigm in languages in which documentation comments can have examples sections; there is typically a tool that runs such examples as unit tests. So since this is a prototype anyway, how about we try the in-source-file tests and see how they feel as the project develops? Of course, if you have a strong preference that the tests be separated into different files from the implementing code itself, I won't block a PR that moves the tests in that way and sets up a convention for where the tests should be located (to me that was a final small motivation for in-source tests; I didn't have to come up with any such convention).

glen commented

2024-10-28 22:34:36 +00:00

Oh and P.S. since it is working for you, I am merging this to main. Then I can proceed with another bit of the Dispatcher implementation.

glen merged commit 047385d56b into main

2024-10-28 22:34:55 +00:00

glen deleted branch test/vitest

2024-10-28 22:34:56 +00:00

glen referenced this issue from a commit

2024-10-28 22:34:57 +00:00

Enable testing with vitest (#13)

josdejong commented

2024-10-31 09:47:17 +00:00

Ah, yes I've see the tests-in-sourcefile approach when I recently tried out Rust.

I'm ok with trying it out (especially inside a POC). I'm totally fine with the idea in general. I doubt whether this will work out nicely in a JavaScript project though. I see that vitest actually supports this concept: In-Source Testing, that's quite neat. I wasn't aware of that.

I think it only works if this way of testing aligns with the language, test framework, and IDE at hand. In JavaScript I think it may give difficulties with correctly keeping imports and code needed for the tests separated from the other imports. You can easily make mistakes there, especially since all IDE's automatically add import statements for you (but at the top of the file). And we need to be careful to ensure that tests are stripped out of the code of the production build. And we need to guard that. I myself am totally happy with a .test.ts file alongside the corresponding .ts, since that guarentees that the test code is separated from the production code and is worry free in that regard.

Anyway, I'm not yet sold but I'm ok with an experiment :)

EDIT: here an interesting article about this, listing pros and cons: https://stevekinney.net/courses/testing/in-source-testing

Ah, yes I've see the tests-in-sourcefile approach when I recently tried out Rust. I'm ok with trying it out (especially inside a POC). I'm totally fine with the idea in general. I doubt whether this will work out nicely in a JavaScript project though. I see that vitest actually supports this concept: [In-Source Testing](https://vitest.dev/guide/in-source.html), that's quite neat. I wasn't aware of that. I think it only works if this way of testing aligns with the language, test framework, and IDE at hand. In JavaScript I think it may give difficulties with correctly keeping imports and code needed for the tests separated from the other imports. You can easily make mistakes there, especially since all IDE's automatically add import statements for you (but at the top of the file). And we need to be careful to ensure that tests are stripped out of the code of the production build. And we need to guard that. I myself am totally happy with a `.test.ts` file alongside the corresponding `.ts`, since that guarentees that the test code is separated from the production code and is worry free in that regard. Anyway, I'm not yet sold but I'm ok with an experiment :) EDIT: here an interesting article about this, listing pros and cons: https://stevekinney.net/courses/testing/in-source-testing

Sign in to join this conversation.

No reviewers

No Label

No Milestone

No project

No Assignees

2 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: glen/math5#13