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

Re-enable integration tests #8405

Merged
merged 3 commits into from
Mar 8, 2023
Merged

Conversation

davidwengier
Copy link
Contributor

@davidwengier davidwengier commented Mar 7, 2023

Fixes #8378 if I'm very very very very very very lucky

@davidwengier davidwengier marked this pull request as ready for review March 7, 2023 23:47
@davidwengier davidwengier requested review from a team as code owners March 7, 2023 23:48
@davidwengier davidwengier changed the title Integration tests Re-enable integration tests Mar 7, 2023
Copy link
Contributor

@ryzngard ryzngard left a comment

Choose a reason for hiding this comment

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

🤞

GenerateCodeBase = true,
OldVersionLowerBound = "4.4.0.0",
OldVersionUpperBound = "4.6.0.0",
NewVersion = "4.6.0.0")]
Copy link
Member

Choose a reason for hiding this comment

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

These being manually maintained feels wrong because there are several error paths here:

  1. As this change shows it's easy to forget individual libraries.
  2. When the integration image updates to a new version we are broken until these values are updated (pretty sure).

Did we ever consider a more automated way of maintaining these?

Copy link
Contributor Author

@davidwengier davidwengier Mar 8, 2023

Choose a reason for hiding this comment

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

Yes, this is very problematic, and both of your points are dead on.

  1. This isn't even "forgetting", this change is because there happened to be an incompatible change in this specific Roslyn DLL, between whats in VS Preview and what we reference, so the list of DLLs here is not exhaustive, its just the ones that have caused problems so far.
  2. Sam has tweaks to this in a PR which look like they will help, so there should be some improvements here I can follow up with, at least as far as version number maintenance goes.

I think the only full "solution" to this, which would allow us to remove the dependencies project altogether is to utilize main-vs-deps, however with EA to Roslyn and potentially tweaks to CLaSP or the Roslyn LSP server, this would likely be permanent, and get used a reasonable amount for regular coding. We'd also need to be strict about only allowing main-vs-deps to reference a Roslyn version that is already inserted into a VS build that is available as an image, so even then our velocity would be severely hampered. Actually, when I was thinking about this, I forgot the "available in an image" bit. That makes it a non-starter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#8410 simplifies this and addresses point 2. If the build is green, hopefully it buys us a reasonable amount of time.

@davidwengier davidwengier merged commit f976f87 into dotnet:main Mar 8, 2023
@davidwengier davidwengier deleted the IntegrationTests branch March 8, 2023 02:01
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.

Re-enable integration tests in release/17.6 and main
4 participants