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

Add basic PlantUML support for feature request #78 (2nd attempt) #86

Merged
merged 28 commits into from
Jun 18, 2021
Merged

Add basic PlantUML support for feature request #78 (2nd attempt) #86

merged 28 commits into from
Jun 18, 2021

Conversation

studix
Copy link
Contributor

@studix studix commented Jun 13, 2021

  • ArchUnitNET.Domain.PlantUml:
    new namespace which contains the PlantUML parser code ported from ArchUnit java

  • TypesShould / TypeConditionsDefinition:
    contains new method AdhereToPlantUmlDiagram for PlantUML based dependency checks

ExampleArchUnitTestPuml demonstrates the feature

studix and others added 27 commits October 20, 2020 21:04
- first test running

Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
@studix
Copy link
Contributor Author

studix commented Jun 13, 2021

The CompilerGeneratedTypesTests is failing on the server, locally it succeeds. I don't see any connection with my changes.

@fgathercoupa
Copy link

fgathercoupa commented Jun 13, 2021

Nice, that looks good :) As for the tests, you are right. Locally I also have a green test. However, something interesting happened when I ran the tests in Debug/Non-Release mode (unfortunately there are subtle differences in what ArchUnit can see depending on Release/Debug compilation). In the Non-Release build suddenly the ClassResidesInMultipleNamespaces test fails.

Xunit.Sdk.EqualException Assert.Equal() Failure ↓ (pos 102) Expected: ···ut is contained in [B, A] Actual: ···ut is contained in [A, B] ↑ (pos 102)

Can you reproduce this?

As for the CompilerGeneratedTypesTests: @brandhuf , I could not reproduce whats happening on Travis. Do you have an idea? Also: Why does the Travis build not catch the ClassResidesInMultipleNamespaces I see locally?

@studix
Copy link
Contributor Author

studix commented Jun 14, 2021

ClassResidesInMultipleNamespaces

When I migrated the ClassResidesInMultipleNamespaces test to use a MemoryStream I noticed an ordering problem when creating the puml diagram in the arrange phase of the test. I fixed the test by changing the assert.

Now that @fgather reports that the order is different on his machine I changed back to use a FileStream instead of a MemoryStream.

@brandhuf
Copy link
Contributor

I ran the ClassResidesInMultipleNamespaces test multiple times and sometimes it fails and sometimes it passes. That's probably why it passed in Travis, while failing on @fgather 's machine.

As for the CompilerGeneratedTypesTests: The error seems to be related to https://docs.microsoft.com/en-us/dotnet/api/system.io.enumeration.filesystementry?view=net-5.0 so maybe it only fails on Linux while passing on Windows? I will try to replicate it.

@brandhuf
Copy link
Contributor

CompilerGeneratedTypesTests fails on my linux machine aswell, I will look into it

@brandhuf
Copy link
Contributor

The error in CompilerGeneratedTypesTests occurs, because FileSystemEntry contains an unsafe fixed char array, which gets compiled into a field with compiler generated field type. The test did not fail in earlier builds, because no type with an unsafe fixed array was referenced in the test assembly. #87 should fix the problem.

@fgather
Copy link
Contributor

fgather commented Jun 16, 2021

Thanks, @brandhuf for quickly fixing this. Of course now that I want to re-test everything, the Travis VM cannot find its dependencies anymore, because Microsoft seems to have decided to rearrange package structures.

@fgather
Copy link
Contributor

fgather commented Jun 18, 2021

the travis env was fixed :) I'll merge and release this. Afterwards we can clean up the equals/gethashcode functions, if we really want to.

@fgather fgather merged commit 3995cdf into TNG:main Jun 18, 2021
@SeMuell
Copy link
Contributor

SeMuell commented Jun 19, 2021

@studix thank you very much for the effort, I highly appreciate it!

@brandhuf it seems that the failing test is still is existent and reproducible on macOS when running debug tests (not in Release). Interesting is, that in Visual Studio Code the test is running in Release by default so I didn't found it out before.

That's the message that I got:

[xUnit.net 00:00:12.65]     ArchUnitNETTests.Domain.PlantUml.ClassDiagramAssociationTest.ClassResidesInMultipleNamespaces [FAIL]
  Fehler ArchUnitNETTests.Domain.PlantUml.ClassDiagramAssociationTest.ClassResidesInMultipleNamespaces [15 ms]
  Fehlermeldung:
   Assert.Equal() Failure
                                 ↓ (pos 102)
Expected: ···ut is contained in [A, B]
Actual:   ···ut is contained in [B, A]
                                 ↑ (pos 102)
  Stapelverfolgung:
     at ArchUnitNETTests.Domain.PlantUml.ClassDiagramAssociationTest.ClassResidesInMultipleNamespaces() in /***/ArchUnitNET/ArchUnitNETTests/Domain/PlantUml/ClassDiagramAssociationTest.cs:line 105

@brandhuf
Copy link
Contributor

Yes, my PR only fixed the failing test in CompilerGeneratedTypesTests. I ran the ClassResidesInMultipleNamespaces test multiple times in debug mode and it sometimes failed and sometimes passed. @studix Can you look into it?

@studix studix deleted the pumlv2 branch June 20, 2021 18:46
@studix
Copy link
Contributor Author

studix commented Jun 20, 2021

I thought this commit would fix it. But it seems that it's unrelated to MemoryStream/FileStream. I will have a look at it.

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.

5 participants