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

Draft application upgrade to SQLAlchemy, Pytest #346

Draft
wants to merge 160 commits into
base: develop
Choose a base branch
from

Conversation

benjwadams
Copy link
Contributor

@benjwadams benjwadams commented May 10, 2024

Draft commit for migration to SQLAlchemy and moves application to application factory pattern for better reusability and testing.

Adds support for BDD testing via pytest-bdd, see example feature files. Example tests extend #343 by implementing these tests.

This is still a work in progress and not complete, but represents a large effort to modernize the DAC architecture.

benjwadams and others added 30 commits April 12, 2024 13:27
Uses application factory pattern.

Moves over to SQLAlchemy from MongoKit.
Removes BerkeleyDB for user authentication, instead using User model
to store password.  FTP PAM authentication methods will need to be
updated to use SQL instead.
Adds a model attribute/DB column for whether CF Standard names
are all compliant after Compliance Checker runs.  Also updates NCEI
archival scripts to utilize this column as part of the criteria
prior to determining whether datasets should be archived by NCEI.
Removes a non-functioning dataset removal task which didn't properly get
queued up due to Flask application context error.  In theory even for
removing many files from the web application from a delayed mode
dataset the operation should not take much time and deletions are not a
common occurrence.  If needed can be fixed and re-added later.
Adds feature file for example scenarios with deployments.
Tests are currently not implemented for these feature files
but will likely be implemented later using pytest-bdd.
Adds BDD testing steps for glider deployment creation, ensuring
that a user can create a deployment successfully, have an email
sent out to Glider DAC group, and ensure the proper filesystem
directory and file structure is created for the dataset.
@benjwadams
Copy link
Contributor Author

pre-commit.ci autofix

@benjwadams
Copy link
Contributor Author

TODO: re-enable unit, BDD tests for test runner.

@benjwadams
Copy link
Contributor Author

benjwadams commented Jan 2, 2025

@ocefpaf, do you know why the unit tests might not be running under GHA? I adjusted the paths to reflect the altered directory hierarchy.

EDIT: resolved, was only running against main and not develop.

@ocefpaf
Copy link
Member

ocefpaf commented Jan 7, 2025

EDIT: resolved, was only running against main and not develop.

@benjwadams can we use main here to reduce the entry level barrier for new contributors who are used to GH defaults?

@benjwadams
Copy link
Contributor Author

Sure, I'll revert that. GHA doesn't seem to be properly installing pytest-bdd in test_requirements.txt despite it being declared as a step. Any suggestions on that?

@ocefpaf
Copy link
Member

ocefpaf commented Jan 16, 2025

GHA doesn't seem to be properly installing pytest-bdd in test_requirements.txt despite it being declared as a step. Any suggestions on that?

I'm not familiar with that docker install steps you are using there. Could it be that the docker run is not preserved in the next step? Try to put the tests in the same step as the installation to check.

Comment on lines 7 to 9
pull_request:
branches:
- main
Copy link
Member

Choose a reason for hiding this comment

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

This should allow this to run on all PRs.

Suggested change
pull_request:
branches:
- main
pull_request:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't realize that was the syntax for it. Tests are now running properly locally so I'll see if applying this change works with GHA.

Copy link
Member

Choose a reason for hiding this comment

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

BTW, I'm not a docker expert but I believe that "docker run" will create a new container and that is why the previous command doesn't persist to the next one. Maybe try to do everything in a single call?

docker run -u root glider-dac-build /bin/bash -c "pip install --no-cache -r /glider-dac/test_requirements.txt && pytest /glider-dac/glider_dac/tests"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this was it. Getting closer now, just need to fix a few paths for the tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants