Skip to content

Conversation

@lg2de
Copy link
Contributor

@lg2de lg2de commented Nov 30, 2020

I was triggered by reading #1353 that most XML assertion messages does not include variable name too.

So I've started reworking these assertions to

  • include variable name
  • provide some useful context information.

While reviewing the texts in the unit tests I found some messages which are misleading because the XmlValidator was following the subject instead of the expectation.

You may want to review the two commits separately.

I'll add changelog when the PR number is available...
Should these two commits result into two statements in the changelog?

@dennisdoomen
Copy link
Member

So I've started reworking these assertions to

Great initiative.

Should these two commits result into two statements in the changelog?

Don't think so. Also, the 2nd commit seems to contain unrelated changes. In other words, you've changed the expected output in the specs, but there are no other changes that are the cause of this.

@lg2de
Copy link
Contributor Author

lg2de commented Nov 30, 2020

The second commit contains change of test and change using expectationIterator instead of subjectIterator.

@dennisdoomen
Copy link
Member

The second commit contains change of test and change using expectationIterator instead of subjectIterator.

Yes, so it's weird that you had to change a test even though nothing changed in that commit. So I fail to understand why you were asking about the commits.

@lg2de
Copy link
Contributor Author

lg2de commented Nov 30, 2020

The commits are doing what their commit message reports:

  1. add variable name and some additional information
  2. change kind of XML validation to get better message

Regarding changelog this could be two changes or a single one.

@dennisdoomen
Copy link
Member

We must be on the wrong channel ;-)

The 2nd commit updates XElementAssertionSpecs.cs, but there's nothing in that commit that seems to cause a change in the assertion method. So I don't understand why that change is in the 2nd commit. If it was caused by the 1st commit, that change should belong there. Just a matter of a fixup commit.

@jnyrup
Copy link
Member

jnyrup commented Nov 30, 2020

Thanks for giving the xml assertions and specs some needed love.

I think the two commits are so closely related, that the single release note covers them both - and that's fine by me.

@dennisdoomen dennisdoomen merged commit 2902362 into fluentassertions:develop Nov 30, 2020
@lg2de lg2de deleted the XmlAssertionMessageImprovements branch November 30, 2020 17:55
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