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

Add some BSP integration tests #3608

Merged
merged 33 commits into from
Oct 8, 2024

Conversation

alexarchambault
Copy link
Contributor

@alexarchambault alexarchambault commented Sep 26, 2024

This adds some BSP integration tests, ensuring the Mill BSP server answers correctly basic BSP requests. BSP responses are serialized on disk and committed, and we compare the requests we get to those on disk. These fixtures on disk can be automatically generated and updated by changing a variable (currently, BspServerUtil.updateFixtures), so that it's quite convenient to generate them, and update them and get a nice diff of the changes at the same time.

@alexarchambault alexarchambault force-pushed the bsp-integration-tests branch 2 times, most recently from 6892849 to 1d70b17 Compare September 26, 2024 17:07
@alexarchambault alexarchambault force-pushed the bsp-integration-tests branch 4 times, most recently from 51659a8 to 4fae5d5 Compare September 30, 2024 16:08
@alexarchambault alexarchambault marked this pull request as ready for review September 30, 2024 16:08
@alexarchambault alexarchambault force-pushed the bsp-integration-tests branch 4 times, most recently from 0f28cf2 to 59d99a8 Compare October 1, 2024 13:03
@alexarchambault
Copy link
Contributor Author

Not sure the CI error is related to my changes

@lihaoyi
Copy link
Member

lihaoyi commented Oct 1, 2024

It's the flakiness that should be fixed by #3642

@alexarchambault alexarchambault force-pushed the bsp-integration-tests branch 2 times, most recently from 9abd3b7 to 8c959eb Compare October 1, 2024 18:52
@alexarchambault
Copy link
Contributor Author

alexarchambault commented Oct 1, 2024

Don't fear the apparent large diff, it's a lot of test fixtures. And most commits are kind of small.

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Great to have some test capabilities. I left some comments below.

bsp/worker/src/mill/bsp/worker/State.scala Show resolved Hide resolved
build.mill Outdated Show resolved Hide resolved
build.mill Outdated Show resolved Hide resolved
os.Path(value)
}

def replaceAllValues(
Copy link
Member

Choose a reason for hiding this comment

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

This name is kind of generic and hard to figure out. How about normalizePathsForTesting?

Copy link
Contributor Author

@alexarchambault alexarchambault Oct 7, 2024

Choose a reason for hiding this comment

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

It's not only paths, it's paths and Java version. I renamed it to normalizeLocalValuesForTesting in the latest push.

@alexarchambault
Copy link
Contributor Author

(If the CI is green, feel free to review, and possibly un-draft and merge)

build.mill Outdated Show resolved Hide resolved
integration/package.mill Outdated Show resolved Hide resolved
@lihaoyi lihaoyi marked this pull request as ready for review October 8, 2024 05:17
@lihaoyi lihaoyi merged commit efa473b into com-lihaoyi:main Oct 8, 2024
24 checks passed
@alexarchambault alexarchambault deleted the bsp-integration-tests branch October 8, 2024 10:03
@lefou lefou added this to the 0.12.0 milestone Oct 8, 2024
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.

3 participants