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

#2130 - Stop skipping certain tests under Windows #6560

Conversation

ineuwirth
Copy link
Contributor

Some tests depended on Unix line ending, changing those to use System.lineSeparator() instead. Now they can run on Windows too.

Some tests depended on Unix line ending, changing those to use `System.lineSeparator()` instead. Now they can run on Windows too.
@ineuwirth
Copy link
Contributor Author

I have noticed in the CI script that android tests are not executed under Windows but it still looked better to keep the sources as close as possible. I guess you would have done it internally anyway.

@cpovirk cpovirk self-assigned this Jun 17, 2023
@cpovirk cpovirk added P2 type=other Miscellaneous activities not covered by other type= labels package=general labels Jun 17, 2023
@cpovirk
Copy link
Member

cpovirk commented Jun 20, 2023

Thanks!

I have noticed in the CI script that android tests are not executed under Windows but it still looked better to keep the sources as close as possible. I guess you would have done it internally anyway.

Perhaps surprisingly, there are several factors here :) The short answer is that we do want to keep them in sync but that it's simplest for you not to bother, as our automation all carry over the changes automatically (at least when they merge cleanly, as they do here).

(When we update both copies, then anyone who runs the Android tests under Windows (even though we don't do it under our CI) gets to run the tests. Plus, as you noted, keeping the sources in sync is itself an advantage.)

Anyway, there's no need to change anything here; I've already pulled this in. And like your other PR, this one will be re-created in another PR with the commit attributed to you.

(FYI, I'm backing out the new usage of String.join to maintain compatibility with ancient versions of Android. I could keep it in the mainline, backing it out only in the backport, but that would cut against the goal of keeping the two in sync.)

@ineuwirth
Copy link
Contributor Author

Thank you for sharing the behind the scenes details about how you handle the android part!

copybara-service bot pushed a commit that referenced this pull request Jun 21, 2023
Fixes #6560

Progress toward #2130

RELNOTES=n/a
PiperOrigin-RevId: 541925496
copybara-service bot pushed a commit that referenced this pull request Jun 21, 2023
Fixes #6560

Progress toward #2130

RELNOTES=n/a
PiperOrigin-RevId: 541925496
copybara-service bot pushed a commit that referenced this pull request Jun 21, 2023
Fixes #6560

Progress toward #2130

RELNOTES=n/a
PiperOrigin-RevId: 541925496
@cpovirk
Copy link
Member

cpovirk commented Jun 22, 2023

And like your other PR, this one will be re-created in another PR with the commit attributed to you.

I'm sorry, I got it right for the other PR but messed it up for this one. (I had some trouble with our automation—related, I theorize but haven't checked, to Windows line endings in the patch!—and ended up putting together the commits myself, only to forget to modify the author line in the one.)

@ineuwirth
Copy link
Contributor Author

No problem at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 package=general type=other Miscellaneous activities not covered by other type= labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants