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

Fluent Assertions 5 upgrade #34

Closed
wants to merge 9 commits into from
Closed

Fluent Assertions 5 upgrade #34

wants to merge 9 commits into from

Conversation

rikrak
Copy link
Contributor

@rikrak rikrak commented Feb 13, 2018

  • Upgraded MVC3 project to .NET 4.5
  • Upgraded #MVC4 project to .NET 4.5
  • Upgraded to Fluent Assertions v5.0.0

Copy link
Member

@kevinkuszyk kevinkuszyk left a comment

Choose a reason for hiding this comment

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

There seems to be a lot in the merge commit 011097c. Can you try a rebase to see if you can drop it?

@kevinkuszyk
Copy link
Member

It also looks like we have some broken tests. Can you take a look at those too?

@rikrak
Copy link
Contributor Author

rikrak commented Feb 13, 2018

Ah. That commit (011097c) was me trying to bring my fork up to date. I shall look into a rebase

@rikrak
Copy link
Contributor Author

rikrak commented Feb 13, 2018

I've had a go at doing a rebase, but I'm not sure if it has made any changes. I'm new to git, so I don't know how to check if the rebase has had the desired effect. It's quite possible I've made things worse :-D

I've re-run the tests locally on a different machine than I used earlier, and they all pass here. Looking at the appveyor failures, it appears that something is not up to date. The failures it is showing are the same that I had after the initial upgrade to FA. I fixed the tests, and committed them (3323e5b), so all should be good.

@kevinkuszyk
Copy link
Member

@rikrak thanks. I've rebased your branch (you can see it here), and you're right the tests run locally, so it looks like there is a problem with AppVeyor.

Leave it with me while look into the AppVeyor issue.

@rikrak
Copy link
Contributor Author

rikrak commented Feb 14, 2018

Thanks for the help with the rebase. I was somewhat stabbing in the dark.
I'd be interested to know what's happened with the AppVeyor build. I'm scratching my head a little as to what the problem could be. If it hadn't fetched the latest source code, it shouldn't build (due to breaking changes in the FA5 API). If it was running against an incorrect version of FA, it would not execute (again due to breaking changes).

Could it be that the AppVeyor environment compiles under a different configuration (.Net 4.5/.Net Standard/etc.) than I use locally, and that the FA5 assembly returns different error messages depending on the configuration?

@rikrak
Copy link
Contributor Author

rikrak commented Feb 20, 2018

I've just looked into this a little further. Not resolved it, but I did find a clue.
Looking at the FluentAssertions code, there's a class called CallerIdentifier[1] that has some conditional pre-processor logic that would produce the result seen in appveyor. It implies, however, that some of the tests are executed with one version of the FluentAssertions assembly (netstandard1.6 or netstandard1.4) and fail, while the rest are executed with another version (net45 or netstandard2.0). I've checked again, and all the projects refer to the net45 version in the project references.

Not sure if this is a red-herring, but it looked like a possible cause of the behaviour in appveyor.
[1] https://github.com/fluentassertions/fluentassertions/blob/5.0.0/Src/FluentAssertions/CallerIdentifier.cs

@kevinkuszyk kevinkuszyk mentioned this pull request Feb 21, 2018
@kevinkuszyk
Copy link
Member

I admit I've not had a lot of time to look into this, but I'm stumped.

I'll open an issue over on the main FA repro, as maybe @dennisdoomen or someone else there might have an idea.

@rikrak
Copy link
Contributor Author

rikrak commented Feb 21, 2018

For Reference: FluentAssertions requires a .NET45 of .NetStandard2 build with debug symbols for Unit Tests in order for the Subject Identification to work. see http://fluentassertions.com/documentation.html#subject-identification

@kevinkuszyk
Copy link
Member

I think if you just switch the unit test projects to build in debug mode for release builds that should fix it.

It looks like re-creating your fork may have disconnected this repro from your fork, so you may need to open a new PR. Sorry about that.

@rikrak
Copy link
Contributor Author

rikrak commented Feb 21, 2018

:-D No worries. It's all good learning for me. I'll have a look at it at lunch time

@kevinkuszyk
Copy link
Member

Superseded by #36.

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.

None yet

2 participants