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

Refactor tests #157

Open
wants to merge 46 commits into
base: master
Choose a base branch
from
Open

Refactor tests #157

wants to merge 46 commits into from

Conversation

jdr0887
Copy link
Collaborator

@jdr0887 jdr0887 commented Dec 1, 2022

Lots of changes in this branch, but mostly contains a refactor of the tests to leverage testcontainers more thoroughly. I also split the code for loading compendia & conflation files into independent CLI apps.

@jdr0887 jdr0887 requested a review from cbizon December 1, 2022 06:40
@jdr0887 jdr0887 marked this pull request as ready for review December 1, 2022 06:41
Copy link
Contributor

@cbizon cbizon left a comment

Choose a reason for hiding this comment

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

Looks good, a few questions though

  • Can you include some docs about how to run the tests? I think I need to fire up a redis or?
  • Does this address /get_semantic_types endpoint is not working #153 ? I saw a test that indicated that perhaps it does
  • It looks like this includes some new conflation abilities. Can you add some doc somewhere about what is required to add more of these? I think this is something we're going to do multiple times in the next year
  • I think that there's a mismatch in startup/shutdown in server with the number of redis connections (we close 7 but open 6)

@jdr0887
Copy link
Collaborator Author

jdr0887 commented Dec 1, 2022

It does address #153 as I refactored loader.py.

I will update the docs.

@gaurav
Copy link
Contributor

gaurav commented Dec 3, 2023

Welp, this PR is extremely out of date.

@jdr0887 @cbizon Any thoughts on how we can incorporate this into NodeNorm as it currently stands?

@gaurav gaurav mentioned this pull request Jul 8, 2024
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