Skip to content

Attempt to fix seemingly test-related bug on Windows #175

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.

Sign up for GitHub

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

Jump to bottom
Closed
jbduncan wants to merge 3 commits into from
Closed

Attempt to fix seemingly test-related bug on Windows #175

jbduncan wants to merge 3 commits into from

Conversation

@jbduncan
Copy link
Member

@jbduncan jbduncan commented Dec 4, 2017 •
edited
Loading

When ResourceHarness.assertFileContents is run on the DBeaver SQL formatter tests, it causes the tests to fail on Windows. This is because the files formatted by the DBeaver SQL formatter end up with CRLF line endings, whereas they're expected to have LF line endings.

However, I could not for the life of me figure out how to change the formatter itself to produce the correct line endings. So I experimented with AssertJ's assertions and discovered that the tests pass on Windows when Assertions.assertThat(actualFile).withCharset(UTF_8).hasContent(expectedContents) is used instead of ResourceHarness.assertFileContents's old implementation.

It's not clear to me if the bug is in ResourceHarness.assertFileContents, AssertJ or in the SQL formatter. However, I've made an educated guess that, if the bug is in the tests and not in the SQL formatter, then it's more likely to be in ResourceHarness.assertFileContents than AssertJ, considering that AssertJ is more thoroughly tested than our own test helpers. Considering this and that the tests start passing on my Windows machine again when AssertJ was used, I think it's probably a bug with assertFileContents's old impl.

I also took the liberty of renaming one of our test helpers, ResourceHarness.assertFile, to assertThatNewFile to make it a bit clearer what happens when an assertion is chained off it.

@nedtwigg
Copy link
Member

nedtwigg commented Dec 4, 2017

I don't like the assertFile / assertNewFile distinction. newFile doesn't actually create a new file, it's just meant as a synonym for new File(testFolder, "path") - usually the file already exists.

If the formatter is producing \r in it's output, that's fine. Lots of third-party libs are going to have wonky newline handling, but Spotless doesn't care about that because we always normalize to \n between steps.

I think the appropriate fix is for StepHarness to normalize to \n.

@jbduncan
Copy link
Member Author

jbduncan commented Dec 5, 2017 •
edited
Loading

I don't like the assertFile / assertNewFile distinction. newFile doesn't actually create a new file, it's just meant as a synonym for new File(testFolder, "path") - usually the file already exists.

Okey dokey, I've reverted the corresponding changes. :)

If the formatter is producing \r in it's output, that's fine. Lots of third-party libs are going to have wonky newline handling, but Spotless doesn't care about that because we always normalize to \n between steps.

I think the appropriate fix is for StepHarness to normalize to \n.

Hmm, I wonder if I've misunderstood you somewhere, because when I revert my initial commit and change StepHarness to normalize the output of the FormatterFunc it is passed to produce LF line endings, the SQL tests now fail to pass on my machine. AFAICT, the SQL tests don't actually use StepHarness, so I'm a bit confused. (See commits 324183d and 5ae5e9f.)

Do you have any further thoughts on this?

@nedtwigg
Copy link
Member

nedtwigg commented Dec 5, 2017

You can normalize line endings with abandon in step tests, because Formatter does that for you when the steps are actually being used.

Ideally, the test infrastructure does this automatically (e.g. StepHarness). If something is being tested outside the infrastructure, that's fine. If slapping a LineEndings.toUnix into a step test fixes it for you, that feels kinda sloppy, but it's fine, because we don't care about that at the step level. (Obviously that's not fine for the tests which are carefully designed to test line ending behavior, but the step-specific tests don't care about line endings).

@jbduncan
Copy link
Member Author

jbduncan commented Dec 19, 2017

I'm very sorry @nedtwigg, for some reason I'm struggling to understand what you're saying. Would you mind rewording things for me?

Just FYI, I'm attempting to refactor the SQL tests ATM to use StepHarness instead of GradleIntegrationTest.gradleRunner(), as they currently do, to see if it resolves these strange test failures on my machine.

@jbduncan
Copy link
Member Author

jbduncan commented Dec 19, 2017

Just FYI, I'm attempting to refactor the SQL tests ATM to use StepHarness instead of GradleIntegrationTest.gradleRunner(), as they currently do, to see if it resolves these strange test failures on my machine.

Actually, never mind. It's not the right time of day for me to tackle this, as it's pretty late in the evening where I live, so I'll tackle this another day. :)

@nedtwigg
Copy link
Member

nedtwigg commented Dec 22, 2017

Have a look at #180 - all tests pass on windows in that PR.

@jbduncan
Copy link
Member Author

jbduncan commented Dec 22, 2017

Wow, looks very good to me @nedtwigg. Many thanks for having a go at it. :)

I'll close this PR then, as I believe it's outdated now.

@jbduncan jbduncan closed this Dec 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jbduncan @nedtwigg