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

[ARCHETYPE-584] don't normalize whitespace in tests so we can test for whitespace #65

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

elharo
Copy link
Contributor

@elharo elharo commented Jul 7, 2021

@newur @Tibor17

Passes in Java 8. I want to see if this fails in Java 11.

@newur
Copy link

newur commented Jul 7, 2021

@elharo
Copy link
Contributor Author

elharo commented Jul 7, 2021

Yep:

Expected: Expected text value '
  ' but was '
    
  ' - comparing <project ...>
  </project> at /project[1]/text()[1] to <project ...>
    
  </project> at /project[1]/text()[1]:
<project>
  <packaging>pom</packaging>
<modules>  <module>myArtifactId1</module>
  </modules>
</project>
     but: result was: 
<project>
    
  <packaging>pom</packaging>
  
  <modules>
      
    <module>myArtifactId1</module>
      
  </modules>
</project>

@elharo elharo marked this pull request as draft July 7, 2021 15:24
@elharo
Copy link
Contributor Author

elharo commented Jul 7, 2021

It might make sense to create a new Java 11 test rather than changing this one.

@newur
Copy link

newur commented Jul 7, 2021

@elharo

I don't see what you try to achieve or what your reasoning is here. However, the call to .normalizeWhitespace() is rather new and I assume it was added to 'get the test green'. So undoing the normalize is bringing the test back to the status quo - which was a stronger test anyway!

You can see it here: bf79618#diff-db4ddee9a40aadf3fab866c350ca950397f69b630de05a126f0265e2136cc070

@elharo
Copy link
Contributor Author

elharo commented Jul 7, 2021

I'm trying to understand what's going on here, and how it fails. This is easier to do one change at a time. This test isn't very well factored to start with, and might need some cleanup before the issue can be fixed.

@newur
Copy link

newur commented Jul 8, 2021

I'm trying to understand what's going on here, and how it fails.

Right, that is always a good approach to solve a bug.

I would hope that the existing PR is a reasonable starting point on how to fix the bug and my comment in JIRA points to the commit that most likely causes the bug.

Anyway, I wish you good luck with the approach you apply to the problem. :)

@elharo
Copy link
Contributor Author

elharo commented Jul 8, 2021

I think you've identified the likely cause of the bug. I do not, however, agree with the proposed fix. There's nothing about this bug that suggests we should be adding schema processing. I'll be quite surprised if that turns out to be necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants