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 harness upgrade #3510

Merged
merged 3 commits into from
Jan 30, 2024
Merged

Test harness upgrade #3510

merged 3 commits into from
Jan 30, 2024

Conversation

jdesrosiers
Copy link
Contributor

I noticed #3503 in my notifications, so I figured I'd do the upgrade real quick. While I was at it, I moved to a nicer test framework.

Honestly, I don't think anyone even remembers these tests exist and they certainly don't ever get run or added to when changes are made to the schema like they should. If this isn't used, it's probably better to just remove it all together. Anyway, this is the upgrade should you choose to keep it around.

@lornajane
Copy link
Contributor

Thanks @jdesrosiers , this is really helpful! I notice that we also now have #3511 and also that since we've been upgrading other dependencies, there's a conflict in package.json. Could you take a look at resolving the conflicts, and we'll try to review it before anything else gets in the way! The tests aren't used enough, but they're all we have and keeping them healthy is greatly appreciated.

@jdesrosiers
Copy link
Contributor Author

@lornajane Done. Rebased and conflicts fixed.

Copy link
Contributor

@lornajane lornajane left a comment

Choose a reason for hiding this comment

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

Thanks! Tests run well, I'm happy to see the update.

@lornajane
Copy link
Contributor

@handrews This pull request overlaps with the other open dependency updates, I suggest we give priority to the human-driven changes (it also includes one update and eliminates another)

@earth2marsh
Copy link
Member

This isn't my area of expertise, but given that the tests are running (thanks for confirming, Lorna!), I think questions would be around what other implications there are around changing the testing framework.

  1. My assumption is that this will minimally impact the workflows or learning curves of potential contributors (and overall should be a net improvement). Is that fair?
  2. Given the mocha tests seem to go back for years, can we expect vitest to be a good choice for at least the next 5 years?
  3. Are there any other implications of this change that should be discussed before approving?

@jdesrosiers
Copy link
Contributor Author

@earth2marsh The primary purpose of this PR was to update the @hyperjump/json-schema library that does the validation check from v0 (no longer supported) to v1. I updated the test framework while I was at it because I thought it would be nice, but if there's any apprehension about the change, I'm happy dropping that commit. It's really not important.

My assumption is that this will minimally impact the workflows or learning curves of potential contributors (and overall should be a net improvement). Is that fair?

Yes, that's fair. Tests are contributed by adding OpenAPI documents to the pass/fail directories, so most contributors won't need to work with this code directly. The tests can be run using the same npm test command that was used previously. The only difference contributors should notice is nicer output when they run tests. If there's a need to make changes to the test harness (which no one has ever done but me), vitest uses the same API as mocha/chai that it's replacing, so there's no learning curve.

Given the mocha tests seem to go back for years, can we expect vitest to be a good choice for at least the next 5 years?

That's fair. Vitest is pretty new. It's built by the people who maintain vite, which is a well established core component of the JavaScript ecosystem. Given the strength of the community behind vite, I don't think vitest is going to disappear anytime soon. But, even if it did, it's not a big deal. The code here is small and that shouldn't change. If this test suite gets used, it will grow by adding OpenAPI documents to the pass/fail directories, not by adding code. It's so little code that it's trivial to change to a different framework for whatever reason. It literally took about a minute to switch to vitest and changing to something else in the future would likely be similarly trivial.

Are there any other implications of this change that should be discussed before approving?

I checked to make sure the framework wasn't being used anywhere else. Changing the framework should not effect anything other than these tests.

@earth2marsh
Copy link
Member

Thank you for confirming--it helped me better understand the reasoning behind the change and its potential blast radius. Even more importantly, thanks for the contribution. 🙏

Copy link
Member

@earth2marsh earth2marsh left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@handrews handrews merged commit e4ddb5e into OAI:main Jan 30, 2024
2 checks passed
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