Skip to content
This repository has been archived by the owner on Nov 29, 2023. It is now read-only.

Type checking and simplification/cleanup of tests #157

Merged

Conversation

eyelidlessness
Copy link
Contributor

What

This change does a few things that aren’t totally inseparable, but have a bunch of chicken/egg characteristics that make it feel more appropriate to land them together:

  • Add static typing to all tests: this is a bit more abrupt than the gradual story discussed in [issue I neglected to file here, but filed in Core]. The reason I've started with tests is that I'm highly motivated to thoroughly understand the existing tests, in preparation for some work that should follow pretty closely behind.

  • Adds some known type definitions1 for libxmljs, which is simultaneously critical and particularly opaque/risky for both maintenance and more forward-looking efforts.

  • Sets up Vite as a build tool for TypeScript, and Vitest as its test runner. I previously converted core and express to ESBuild, which Vite uses for most transpilation duties. It turns out that Vite is simpler to set up (a breath of fresh air in JS ecosystem build tooling), and Vitest being so trivial to integrate with nearly no config fuss is a big testament to that. Vite also has some very compelling features around targeting multiple environments. For today, what that means is that while I deferred converting the user-facing portion of transformer (in this PR; that will follow shortly, but this one is already big enough), it should be pretty straightforward to handle the build and distribution portions of that effort.

  • Simplifies test code in a variety of ways. Mostly this is chore work cleaning up copypasta and patterns that make tests harder to follow (e.g. converting Promise API calls to async/await, direct property access rather than an opaque chai extension). A smaller portion, but one I think will be more valuable in coming days, isolates some platform-specific stuff.

Why [now]?

Partly because this accomplishes some of what I’ve wanted to do to be a better maintainer in the long term. If I can better understand the code, I can better address user needs, and find root causes of bugs more quickly. Static typing helps with that immensely, and means I can do future work more confidently with less risk of introducing regressions. If I called it a year on this PR alone, I’d feel happy about wrapping up on that note. But there is more to come!

Footnotes

  1. Well, relatively safely inferred from usage.

@eyelidlessness eyelidlessness force-pushed the refactor/simplified-typed-tests branch from 7247d7a to ad0529b Compare December 24, 2022 23:35
*
* 1. Does export it.
* 2. Is a drop-in replacement for `@xmldom/xmldom`.
* 3. Could very possibly go away soon anyway ;)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a holiday already in some parts of the world, so I put off some commit history hygiene which will hopefully make this change as reviewable as possible. But in the spirit of it being a holiday, I left this little gift.

@eyelidlessness eyelidlessness force-pushed the refactor/simplified-typed-tests branch from ad0529b to 0b1c462 Compare December 25, 2022 18:12
@eyelidlessness eyelidlessness force-pushed the refactor/simplified-typed-tests branch 3 times, most recently from 0235d02 to b115307 Compare December 25, 2022 18:41
... as a separate commit so the diff makes more sense
+ fix type errors
+ a variety of cleanup and consistency across test modules
+ trivial conversion of sheets.spec.js to TypeScript as well

# Conflicts:
#	test/transformer.spec.ts
- Promise.all around assertions
- .to.have.property('foo').and... (these were previously .to.eventually...)
@eyelidlessness eyelidlessness force-pushed the refactor/simplified-typed-tests branch from b115307 to 623731f Compare December 25, 2022 18:46
@eyelidlessness
Copy link
Contributor Author

I'm not sure how much better the diff is going to get at the pull request level, but I've tried to at least make sure that file renames are part of the commit history and that you can at least view diffs for each file by commit, rather than a deleted and added file. If I've missed any and it would hinder review, I'm happy to give it another shot.

The history still isn't necessarily ideal, particularly where bad-external.spec.js was changed to use shared logic while also converting transformer.spec.js. Those changes felt logical at the time but may not be great for review.

So, in general, if it would help review to further revise the commit history I'd be happy to do that. But I'm hesitant to get more bogged down in that without direction from a reviewer.

eyelidlessness added a commit to eyelidlessness/enketo-transformer that referenced this pull request Dec 26, 2022
(I've learned my lesson from enketo#157, hopefully this will make it easier to review!)
@lognaturel
Copy link
Contributor

The TODO in 9b1dc9a is no longer relevant, right? It looks like it was easy to keep the badge and if it is I think we might as well keep it.

I went through each commit to make sure I understood the general nature of the changes introduced and the reason for them. I did NOT verify the types myself and did NOT read through every single transformation. I paid special attention to bigger sections of diffs or diffs within a file that didn't match an established pattern. Overall it seems unlikely coverage has been lost or changed and this is looking ready to merge to me.

@eyelidlessness eyelidlessness force-pushed the refactor/simplified-typed-tests branch from 623731f to 0da3c32 Compare January 3, 2023 23:39
eyelidlessness added a commit to eyelidlessness/enketo-transformer that referenced this pull request Jan 3, 2023
(I've learned my lesson from enketo#157, hopefully this will make it easier to review!)
@lognaturel lognaturel merged commit ad621e5 into enketo:master Jan 4, 2023
eyelidlessness added a commit to eyelidlessness/enketo-transformer that referenced this pull request Jan 13, 2023
(I've learned my lesson from enketo#157, hopefully this will make it easier to review!)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants