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

fix integration tests #243

Merged
merged 26 commits into from
Jun 7, 2024
Merged

fix integration tests #243

merged 26 commits into from
Jun 7, 2024

Conversation

c-cube
Copy link
Collaborator

@c-cube c-cube commented Apr 1, 2024

these tests need updating after the changes in how single-oneof messages are translated.

@c-cube c-cube force-pushed the simon/fix-integration-tests branch from 2b40153 to c15fc69 Compare April 1, 2024 03:57
@c-cube c-cube changed the base branch from master to br-4.0 April 1, 2024 04:00
Copy link
Contributor

@Lupus Lupus 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 overall. Having those integration tests run in CI will guarantee they do not get silently broken.

@@ -1,3 +1,5 @@
(env
(_
(flags :standard -warn-error -a+8 -w +a-4-40-41-42-44-48-70)))

(data_only_dirs runtime-bs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could just drop it altogether?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe, it doesn't cost much right now, idk. I'm not sure how to inquire about the existence of current users?

Copy link
Contributor

Choose a reason for hiding this comment

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

How is commenting it out via data_only_dirs different from just removing? Git has history in place in case it needs to be resurrected - it would be trivial to do so... Not sure about how to inquire about the existence of current users. We can probably find out who introduced that part of the codebase and check with them?

src/tests/unit-tests/bytecode/dune Show resolved Hide resolved
@c-cube
Copy link
Collaborator Author

c-cube commented Jun 7, 2024 via email

@c-cube c-cube force-pushed the simon/fix-integration-tests branch from 30bdbc8 to 45a3e21 Compare June 7, 2024 15:48
@Lupus
Copy link
Contributor

Lupus commented Jun 7, 2024

Fair enough, I guess we can remove it. The old package will be there, we just don't support it in the next major release?

Yeah, sounds like a plan

@c-cube
Copy link
Collaborator Author

c-cube commented Jun 7, 2024

Ah damn, I remember now, I got stuck on fixing the CI because the google files are not there 😢 : https://github.com/mransan/ocaml-protoc/actions/runs/9419796009/job/25950383987#step:11:41

@Lupus
Copy link
Contributor

Lupus commented Jun 7, 2024 via email

@c-cube
Copy link
Collaborator Author

c-cube commented Jun 7, 2024

Maybe we can vendor them just for CI purposes, here?

@c-cube c-cube force-pushed the simon/fix-integration-tests branch from f2e90ac to 9672ed0 Compare June 7, 2024 19:10
@c-cube
Copy link
Collaborator Author

c-cube commented Jun 7, 2024

ALRIGHT IT BUILDS!!! 😵‍💫

@c-cube c-cube merged commit 2e36f5d into br-4.0 Jun 7, 2024
5 checks passed
@c-cube c-cube deleted the simon/fix-integration-tests branch June 7, 2024 19:35
@Lupus
Copy link
Contributor

Lupus commented Jun 7, 2024

ALRIGHT IT BUILDS!!! 😵‍💫

Sounds like CI pain 😁
Great that you pushed this through!

@c-cube
Copy link
Collaborator Author

c-cube commented Jun 7, 2024

ahah yeah, I moved some stuff to docker so I could run it locally first 😂 . I think we're getting in better shape for 4.0!

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