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 #34

Merged
merged 24 commits into from
May 12, 2021
Merged

Add unit tests #34

merged 24 commits into from
May 12, 2021

Conversation

fereira
Copy link

@fereira fereira commented May 12, 2021

Adding this PR to create unit tests for Lookup utils, MarcUtils, and ApiServices...these are just basic tests for now that can be built upon

@fereira fereira requested a review from cappadona May 12, 2021 13:02
Copy link

@cappadona cappadona left a comment

Choose a reason for hiding this comment

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

Thanks @fereira. This is looking good. A few questions:

  1. Did we get a 👍🏼 from @starsplatter to commit the MARC files? They don't appear to be sanitized
  2. If I remove -DskipTests=true from the Dockerfile, all tests fail when building the Docker image. Do you want me to push a commit that adds test support for the containers or do you prefer to keep that in a separate PR?

Copy link
Member

@starsplatter starsplatter left a comment

Choose a reason for hiding this comment

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

sanitized

@fereira
Copy link
Author

fereira commented May 12, 2021

I hadn't tested it in a container. I prefer not to use containers on my development machine for developing code. I'd like to get tests merged so that I can work on other issues and have tests that I can work on for the new issues (e.g. the map notes issue).

@cappadona
Copy link

cappadona commented May 12, 2021

Sounds good. We'll leave it as is for now and won't have tests run as part of the deploy. Feel free to merge when you're ready.

I can open a new PR that adds container support for tests when/if we so desire

@fereira fereira merged commit 503cd8b into main May 12, 2021
@fereira fereira deleted the add-unit-tests branch May 12, 2021 22:19
@fereira
Copy link
Author

fereira commented May 12, 2021

Do you know if there is a "standard" way to put unit tests which require external files to execute? I'm wondering if part of the deploy process would be to copy them to a "named" volume in the container that's referenced by an ENV variable.

@cappadona
Copy link

We can just copy them to the build artifact so that maven can run the tests. I opened a draft PR so you can see for yourself.

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