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

Adds test that validates that older supergraphs can be read by new ve… #2297

Merged
merged 1 commit into from
Dec 14, 2022

Conversation

pcmanus
Copy link
Contributor

@pcmanus pcmanus commented Dec 12, 2022

…rsions

Because newer versions can't generate old subgraphs, this commit creates a new scripts that allow to easily generate a supergraph for the "current" version. The intent is that with each new version, we'll generate the corresponding supergraph and commit it to source control. As such, any following version will have older versions of the supergraph which it can test the reading of.

This is imperfect as this only test a relatively specific supergraph, and making change to it would be painful since we've have to backport the change to older versions and re-generate new versions of the supergraph (possible, but painful). But it is better than nothing as it will quickly catch any obvious breaking in the reading of older supergraphs.

…rsions

Because newer versions can't generate old subgraphs, this commit creates
a new scripts that allow to easily generate a supergraph for the
"current" version. The intent is that with each new version, we'll
generate the corresponding supergraph and commit it to source control.
As such, any following version will have older versions of the
supergraph which it can test the reading of.

This is imperfect as this only test a relatively specific supergraph,
and making change to it would be painful since we've have to backport
the change to older versions and re-generate new versions of the
supergraph (possible, but painful). But it is better than nothing as
it will quickly catch any obvious breaking in the reading of older
supergraphs.
@netlify
Copy link

netlify bot commented Dec 12, 2022

👷 Deploy request for apollo-federation-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 5da3b50

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@pcmanus
Copy link
Contributor Author

pcmanus commented Dec 12, 2022

Going to mark this draft to convey the fact that I'm not entirely convinced this is a good approach.

But essentially, the problem we have is that newer versions of the query planner should be able to read and use older supergraphs, but we don't really have any test for that. Particularly as we're about to bump the join spec version to 0.3 in the next release, having tests that ensure we don't completely break when reading 0.2 versions would be nice.

But setting up that kind of tests is a bit painful as we need to keep around a bunch of older supergraphs that newer versions can test against and:

  • as we move forward and add more and more versions, it would be nice to avoid having the process of generating those supergraphs to keep around and test against not too manual, or it'll fall by the wayside.
  • it makes it very hard to change the tests themselves, and doing so would easily mean re-generating the supergraph for old the versions we want to test against, which isn't trivial.

In any case, this PR is some attempt at having "something". It tries to tackle the problem of the 1st bullet point above: it makes it easy to generate a test supergraph from the current version and to keep it around. In theory, if we don't forget to generate a new test supergraph on new versions (this part is still manual), we'd accumulate older supergraphs that never versions can test against (which is why this patch is against main btw; the idea is that we'd generate a supergraph against the 0.2 join spec and commit it before merging main into next, at which point we'll have a 0.2 supergraph to test against the newer code).

Overall, I'm on the fence on whether it's worth having something like this in-tree. Maybe this kind of backward compat is better left entirely to our offline harness.

`
}

const reviews = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would separate the generation code and the tests into separate files, but it's up to you.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see you address it in the comment below. Again, it's fine if you don't think it's worth the hassle.


// Ensure we're not given a completely broken argument. This also reject any `-alpha` or `-rc` versions on purpose: no
// point in keeping test supergraphs for those versions around.
if (!(/[0-9]+\.[0-9]+(\.[0-9]+)?/.test(version))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't actually check that the file we are trying to generate is the branch that we're currently on. Probably nbd, but I thought I'd call it out as a potential footgun.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah; I wasn't actually sure how to do something useful here without being complicated. In theory, I wanted to use the current branch version instead of asking for a version manually, but as far as i can tell, we only bump versions just before releases, so the version of the "current branch" is usually the version of "whatever was the last release", and so it's not exactly the version we'd want for this.
I figured passing the version manually might end up less frustrating that something that tries to be automatic but does the wrong thing, but I'm open to suggestions (maybe someday we get this integrated to the release process...). Anyway, I'll note that we still need to manually commit the result of this script, which give more change to double check for mistakes.

@pcmanus pcmanus self-assigned this Dec 14, 2022
@pcmanus pcmanus marked this pull request as ready for review December 14, 2022 14:44
@pcmanus pcmanus merged commit 195732e into apollographql:main Dec 14, 2022
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.

2 participants