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

main: move YAML examples to test fixtures, remove examples folder #4166

Merged
merged 3 commits into from
Nov 1, 2024

Conversation

ralfhandl
Copy link
Contributor

@ralfhandl ralfhandl commented Oct 29, 2024

  • Move examples/v3.1/*.yaml files to tests/v3.1/pass/
    • These files are automatically processed by tests/v3.0/schema.test.mjs
    • Remove tests/v3.0/examples.test.js as it does nothing now
  • Move examples/v3.0/*.yaml files to tests/v3.0/pass
    • Adjust tests/v3.0/examples.test.js to read from this folder
    • Rename tests/v3.0/examples.test.js to schema.test.mjs
  • Remove examples folder
  • Remove workflow convert-examples-to-json.yaml

@ralfhandl ralfhandl marked this pull request as ready for review October 29, 2024 10:16
@ralfhandl ralfhandl requested review from a team as code owners October 29, 2024 10:16
@handrews
Copy link
Member

@ralfhandl are these the same examples we moved to the Learn site? If so, would it be better to check them out from there and only maintain them in one place?

@ralfhandl
Copy link
Contributor Author

I like to have tests right next to the code, meaning in the same branch of the same repo, and that's where they are now.

That they happen to be identical to something that is also living somewhere else doesn't matter, the two locations serve different purposes and I would not be surprised or concerned if they evolve into different directions.

@handrews
Copy link
Member

@ralfhandl fair enough. I think this ties into my questions in #4154 about the purpose of each branch, and clarity on what is staying in this repository and what needs to move. Both that change and this one, and several others we have discussed, will be so much easier to agree on and approve if we have a coherent framework.

I'm struggling with these things popping up separately because I can't visualize a coherent end state. I don't really want to block this as I agree it's an improvement, but I'd like to plot the simplest path from where we are to where we want to be. That includes figuring out where our infrastructure lives.

With three specs that should use the same build, publish, and test infrastructure, all of the code / infrastructure should live in a separate neutral space as much as possible. But the test data that is specific to a spec should live with the spec.

So I might actually be ending up at a different perspective here: I think test infrastructure and test data probably live in different places as they change for different reasons at different times. But I don't actually quite know what constitutes "test infrastructure" and what pieces can be spec-neutral and what pieces cannot. And I'd like to understand that before rearranging things.

@ralfhandl
Copy link
Contributor Author

I understand your concerns, and I think this PR does not interfere with whatever we will come up with in the future. It merely finishes what we began with OAI/learn.openapis.org#102 almost half a year ago: move (not just copy) the examples to the learn repo.

It also preserves and simplifies the schema tests, which we will need anyway in the future.

Wherever we go from here, this cleanup is useful and can be done now.

@ralfhandl ralfhandl requested a review from a team October 31, 2024 17:17
Copy link
Contributor

@mikekistler mikekistler left a comment

Choose a reason for hiding this comment

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

Looks good. 👍

@ralfhandl ralfhandl merged commit 20fe7a7 into OAI:main Nov 1, 2024
2 checks passed
@ralfhandl ralfhandl deleted the main-move-examples-to-test-fixtures branch November 1, 2024 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants