Add unit tests for "staying on the same page when switching versions" functionality #7338
Conversation
ilyagr
force-pushed
the
test
branch
6 times, most recently
from
3446e33
to
651b145
Compare
|
Thanks! We'll check it out once we consider adding tests. Please understand that we're currently working on other, much bigger things in the background, so please be patient until we consider this ready for merging. Thank you! |
ilyagr
force-pushed
the
test
branch
6 times, most recently
from
4c7f11c
to
66a844f
Compare
|
I now updated this PR to follow your suggestions from #7350 (comment) as best I can. I added a commit that switches Mocha -> Jasmine and moves the tests to a separate dir. I also marked it as ready for review. Once/if this PR is merged, I'll update #7350 accordingly. At that point, it will become a much smaller PR with only business logic updates. The suggested As you might be able to tell, I'm no expert on TS testing frameworks; I simply use whatever I can get to work and makes minimal changes to the project structure (so that it can be replaced later, if needed). OTOH, let me know if there's more I can do to make this easier for you to review/maintain. I'm trying to make the minimal changes to the project structure possible, so that it's easy to change anything about this setup later. |
| * selector. | ||
| * | ||
| * @param selectedVersionSitemap - as obtained by fetchSitemap from `${selectedVersionBaseURL}/sitemap.xml` | ||
| * @param selectedVersionBaseURL - usually `${currentBaseURL}/../selectedVersion` |
There was a problem hiding this comment.
I am not sure what to do about the eslint warning here, it seems to be a bug in the jsdoc linter.
One possibility is to silence the warning (not sure how).
Another is to have a single params argument and move the documentation to the type.
I do like having an object as the parameter, as it makes the tests much more readable.
|
I noticed that you marked this as a draft. That's fine, but just to be clear, I wasn't planning to make any further changes here, except for maybe occasionally rebasing it on top of the current master from time to time, until I hear from you. |
|
Alright, thanks! |
…tching versions"
We will add unit tests for this functions in the next commit.
The function gets its own file because I was unable to get the test runner ("mocha") to work otherwise. See the child commit's description for more details.
… functionality This is meant to simplify checking correctness of code like squidfunk#7227 and fixing bugs like squidfunk#7226. In fact, I'm hoping to eventually make this code general enough that it runs locally with `mike serve`. I picked `mocha` as the testing library because its Typescript support relies on ts-node, which this project already relies on. I have little experience with `mocha` vs `jest` vs something else beyond this commit. See also https://github.com/mochajs/mocha-examples/tree/main/packages/typescript Instead of using `chai`, I'm using Node's assert package. Its TS types are in `@types/node`. It should be trivial, and might be good, to switch to `chai` or something else. This setup is not perfect, in particular I can't get tests to import the whole `index.ts` file. This is why the function being tested gets its own file. Importing `index.ts` would seem to require writing some DOM shims and be a general headache. For the record, using `tsx` and `jsdom-global` instead of `ts-node` to run `mocha` seemed like the most promising approach, but it still failed since some files imported from this `index.ts` rely on the DOM having some particular structure in their top-level definitions. Here's how to set up `tsx` and `jsdom-global`, for reference: ```diff diff --git a/.mocharc.json b/.mocharc.json index e713305556...33c9adc84d 100644 --- a/.mocharc.json +++ b/.mocharc.json @@ -2,5 +2,5 @@ "$schema": "https://json.schemastore.org/mocharc.json", "extension": ["ts"], "spec": "src/**/**.test.ts", - "require": "ts-node/register" + "require": ["tsx", "jsdom-global/register"] } diff --git a/package.json b/package.json index ecc9f98cf9...10242ca725 100644 --- a/package.json +++ b/package.json @@ -76,6 +76,7 @@ "gitlab": "^14.2.2", "google-fonts-complete": "jonathantneal/google-fonts-complete", "html-minifier": "^4.0.0", + "jsdom-global": "^3.0.2", "material-design-color": "^2.3.2", "material-shadows": "^3.0.1", "mocha": "^10.6.0", @@ -99,6 +100,7 @@ "svgo": "3.0.0", "tiny-glob": "^0.2.9", "ts-node": "^10.9.2", + "tsx": "^4.16.2", "typescript": "^5.5.2" }, "engines": { ``` I can also incorporate the diff into this commit.
I tried to follow the suggestions from squidfunk#7350 (comment) as best I can. The suggested `iframe-worker` template uses Karma, but I didn't use it because 1) it's a complicated tool I don't understand and 2) it says it's deprecated and isn't even accepting bugfixes in https://github.com/karma-runner/karma Instead, I followed https://www.innoq.com/en/blog/2020/04/ts-jasmine-karma/#executingtestsonnode.js to set up Jasmine with ts-node, since that's all I need for this application. If you need something like Karma or whatever replaced it, you can always rework this later. OTOH, let me know if there's more I can do to make this easier for you to review/maintain. I'm trying to make the minimal changes to the project structure possible, so that it's easy to change anything about this setup later.
|
Feel free to take a look (it's now synced with But let me know if this is not necessary and you can review this as is, it'd make things a bit easier for me. |
|
Judging by #7350 (comment), the updates I planned wouldn't make any difference to the fate of this PR. So, I'll leave it as is. I still think it's a good idea, so I'll leave it open, but do with it as you will. After all, it's just a contribution made with the hope that it helps. |
This is squidfunk#7350 with the tests removed, as requested in squidfunk#7350 (comment) . I do urge you to reconsider and merge something like squidfunk#7350 (including the tests I wrote) instead. Without tests, I suspect this code will break like its [previous version did]( squidfunk@ceacfe5) . Once the code does break, it might be difficult to fix without tests. Even if other parts of the theme work without tests, and even if you aren't in love with how I set up the infrastructure in squidfunk#7350 or squidfunk#7338, I think this logic is complex enough to need tests to be maintainable. Again, I wouldn't be able to check this code's correctness without the tests that aren't included in this PR. Of course, this is your project to maintain, so do what works best for you. I just hope this bugfix helps. --------------------- Fixes squidfunk#7226 Additionally, this allows using the version switcher even if the website is published at a different URL than the `site_url`. In particular, the version switcher should now work locally when serving the site with `mike serve`.
This is squidfunk#7350 with the tests removed, as requested in squidfunk#7350 (comment) . I do urge you to reconsider and merge something like squidfunk#7350 (including the tests I wrote) instead. Without tests, I suspect this code will break like its [previous version did]( squidfunk@ceacfe5) . Once the code does break, it might be difficult to fix without tests. Even if other parts of the theme work without tests, and even if you aren't in love with how I set up the infrastructure in squidfunk#7350 or squidfunk#7338, I think this logic is complex enough to need tests to be maintainable. Again, I wouldn't be able to check this code's correctness without the tests that aren't included in this PR. Of course, this is your project to maintain, so do what works best for you. I just hope this bugfix helps. --------------------- Fixes squidfunk#7226 Additionally, this allows using the version switcher even if the website is published at a different URL than the `site_url`. In particular, the version switcher should now work locally when serving the site with `mike serve`.
This is squidfunk#7350 with the tests removed, as requested in squidfunk#7350 (comment) . I do urge you to reconsider and merge something like squidfunk#7350 (including the tests I wrote) instead. Without tests, I suspect this code will break like its [previous version did]( squidfunk@ceacfe5) . Once the code does break, it might be difficult to fix without tests. Even if other parts of the theme work without tests, and even if you aren't in love with how I set up the infrastructure in squidfunk#7350 or squidfunk#7338, I think this logic is complex enough to need tests to be maintainable. Again, I wouldn't be able to check this code's correctness without the tests that aren't included in this PR. Of course, this is your project to maintain, so do what works best for you. I just hope this bugfix helps. --------------------- Fixes squidfunk#7226 Additionally, this allows using the version switcher even if the website is published at a different URL than the `site_url`. In particular, the version switcher should now work locally when serving the site with `mike serve`.
This PR is the no-op part of #7350, mostly consisting of adding tests. Once this is merged, I'll rebase #7350 on top of the merge, and it will become a much smaller PR only affecting business logic.
This is meant to simplify checking correctness of code like #7227 and fixing bugs like #7226. In fact, I'm hoping to eventually make this code general enough that it runs locally with
mike serve.See the commit descriptions for more details.