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

Implement tests #20

Merged
merged 1 commit into from
Nov 14, 2019
Merged

Implement tests #20

merged 1 commit into from
Nov 14, 2019

Conversation

coderpatros
Copy link
Member

This is a work in progress and part of a larger effort to address #11

There is still some work to make this useful. But wanted to get your opinion on this before going too much further.

The basic plan is to implement an independent service for directory, project and solution analysis as well as NuGet in a way that makes them easy to test.

@coderpatros
Copy link
Member Author

@stevespringett this is turning into a much larger change than I thought. Just want to touch base and make sure you are happy for me to continue.

I was hoping I could make this in smaller changes. But I think one big change might be easier to work through. Happy to take on a co-maintainer role too. Well once there is good test coverage.

@stevespringett
Copy link
Member

I like the refactoring you're doing. It's a bit hard to follow the changes, but the overall design is good. So much better that what I had, for sure.

@coderpatros
Copy link
Member Author

Thanks @stevespringett I'll keep working on it then. Once I have it in a semi-finished state I'll let you know. It's still bit of a dog's breakfast. But once complete it should be a bit easier to review.

@coderpatros
Copy link
Member Author

@stevespringett this still needs some work. Mainly around info, debug, etc console output. And error handling. But the structure shouldn't change too much (subject to your feedback of course). But I think this is ready-ish for your feedback. Although happy for you to hold off until I think it is ready to merge. It is a ridiculously big commit. So it will take a fair bit of your time. Happy to hook up on a slack call if it helps.

Should probably re-work the ProgramTests.cs in the future. Ideally there would be separate integration tests that flex the entire command line pipeline. And some program unit tests that use mocked services. But I think this provides reasonably good test coverage.

There is a script with the tests test-coverage.sh. If you run dotnet tool install -g dotnet-reportgenerator-globaltool first you'll be able to run the tests and generate a coverage report.

I don't use Travis CI myself. So I don't really know where to start with hooking up build tests and test coverage. Hoping you do. If not I'm happy to investigate it some more.

@coderpatros
Copy link
Member Author

Actually, now that it’s closer to done I should be able to split this up into more manageable commits.

@stevespringett
Copy link
Member

Slack (on the OWASP workspace) might be good just to walk me through it. Regarding Travis, I'm in the process of changing everything over to GitHub Actions.

https://github.com/CycloneDX/cyclonedx-dotnet/actions

Once merged in, I'll need to modify the action to ensure the tests are run. Also, I have some project and solution files that I test with locally. Would these be beneficial?

@coderpatros
Copy link
Member Author

@stevespringett sounds good. What days/time suit you? And what timezone are you in?

If you can send me the project and solution files you test with locally I'll add them to the tests. I'm @patros on the OWASP Slack too.

@coderpatros
Copy link
Member Author

@stevespringett just bumping this Steve. Can you send me those test files?

@stevespringett
Copy link
Member

Examples.zip

Thanks for the ping. Attached are some of the test files I use to validate 'special cases'. Ideally, we could grow these to support projects with hundreds of nuget packages.

@coderpatros
Copy link
Member Author

Hi @stevespringett I have made use of those files as three new test cases using currently generated output as the expected test result:

  • CallingCycloneDX_WithNoPackages_GeneratesEmptyBom (solution file and csproj file with no package references)
  • CallingCycloneDX_WithNoProjects_GeneratesEmptyBom (solution file with no project references)
  • CallingCycloneDX__GeneratesBom (solution file with references to csproj with package references, csproj without package references and vbproj with package references)

Not sure how many different combinations of those files you use for testing. So let me know if there is a particular case I've missed that you'd like covered.

I've put these ones under an "IntegrationTests" project (although it still uses a mocked file system).

I think this is pretty much good to go if you want to try and organise a time to go through it together. Although I still need to run it against all our repos and compare to the current output. We have 270 repos, with a combination of old & new .net framework, .net standard and .net core projects. So they should give it a good flex and tease out any funny corner cases.

@coderpatros
Copy link
Member Author

@stevespringett turns out I've introduced some slightly different behaviour in this PR.

It appears that currently not all transitive dependencies are being resolved. Now it will keep going so that all packaged dependencies will be included regardless of how deep. IMHO this is correct behaviour. But judging by issue #15 this isn't what everyone expects. But I could add another parameter to adjust this behaviour in a subsequent PR.

I also wanted to validate that I hadn't introduced a performance regression. Turns out performance has been improved. Previously the same nuget package would be resolved multiple times per project and per run under certain circumstances. Our internal BOM generation process has gone from approx 1.5 hours to 1 hour.

I know you're probably busy, and this is an absolute monster of a PR, but it would be great if you can find time to review this. There's some additional functionality I'm pretty keen to get started on but want to base it on whatever this ends up being merged as.

@coderpatros coderpatros changed the title Implement tests - work in progress Implement tests Nov 6, 2019
@coderpatros
Copy link
Member Author

Also, don't worry about hurting my feelings or anything like that. This really is a monster PR. If you simply can't accept such a big change in one PR let me know.

@stevespringett
Copy link
Member

It appears that currently not all transitive dependencies are being resolved

The correct behavior should be to include all direct and transitive effective dependencies. Effective dependencies are different from stated dependencies. #15 is the result (I believe) of recursively going through components stated dependencies and adding them to the BOM. This resulted in incorrect versions in the BOM. To correct this, the code should rely on the same mechanism that nuget uses to resolve effective dependencies, and only include those in the resulting bom.

@coderpatros
Copy link
Member Author

Yeah, the current dependency resolution logic is pretty basic. I'll be looking at swapping in the standard Nuget libraries to do this in the future.

@stevespringett stevespringett merged commit 19acd28 into CycloneDX:master Nov 14, 2019
@coderpatros coderpatros deleted the tests branch February 5, 2020 13:01
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