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 unit tests for Container AddComponent. #343

Merged
merged 6 commits into from
Jan 22, 2023
Merged

Conversation

vernonr3
Copy link
Contributor

Provides unit test of function:
Adding existing component returns existing
Adding new component returns appended component
Adding component that matches name but has update fields tests the merging of those fields. With exception of DSLFunc. Not quite sure why the merging function doesn't work here.
Adds script to assess unit test coverage and html output to get colour coded output.
States unit coverage of file now to be 93.3%..

I haven't sought to minimise the lines of test code & initialisation - aiming to be clear rather than clever.

Initialization has been written out long hand. Better being clear rather than concise and too clever for people to see what is being tested
The provides almost all of the necessary unit testing for this function

TBD: Not succeeded in getting the merging of two different anonymous DSLFunctions.
Provides complete coverage except for merging of DSL functions.
Work in progress many more tests to add
@raphael
Copy link
Member

raphael commented Jan 22, 2023

This is great, thank you for the nice contribution! A few things before we can merge it:

  • The tests fail because of linting issues. You should be able to see them locally by running make or make lint.
  • Test coverage should already be reported via Foresight, for example in this build. The change impact analysis is empty though, so something isn't quite right - I'll look into it. Still, we probably shouldn't add assesscoverage script and tmp directory to the repo.

Thanks again, I'll merge once the build passes, and the test coverage stuff is removed. Ideally, we'd also remove the commented-out test (or enable/finish it).

@vernonr3
Copy link
Contributor Author

Thanks Raphael for your warm and quick response.

  • I've sorted the linting issues
  • Removed the assesscoverage and tmp directories from the repo. I'll use them locally as it gives nice coloured feedback on progress
  • I've also completed the commented out test and added another. Do you want a separate PR for this?
    Thanks Vernon

@raphael
Copy link
Member

raphael commented Jan 22, 2023

This is great, no need for a separate PR. Thank you!

@raphael raphael merged commit cf05027 into goadesign:main Jan 22, 2023
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.

2 participants