Skip to content

Integration tests and Refactoring#1

Open
helena-donaldson wants to merge 10 commits into
mainfrom
experiment_submission_tests
Open

Integration tests and Refactoring#1
helena-donaldson wants to merge 10 commits into
mainfrom
experiment_submission_tests

Conversation

@helena-donaldson

Copy link
Copy Markdown
Collaborator

This PR includes the following:

  • The addition of an integration test script
  • The reorganization of test related materials into their own directory, with separate unit tests and integration tests directories, with each having their own subdirectory for data that their tests rely on
  • Addition of cleanup logic that deletes generated files that are produced during the testing process (artifact files that only serve to clutter the directory)

@helena-donaldson helena-donaldson marked this pull request as draft April 7, 2026 04:23
@helena-donaldson helena-donaldson marked this pull request as ready for review April 7, 2026 04:26

@rhit-fruechss rhit-fruechss left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Mostly good - the README for the tests is especially welcome. There are just a couple of minor maintainability issues.

import unittest
import zipfile

sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), '../..')))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This line should either be split into multiple lines or have a comment pointing out that it imports the GLADOS CLI.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Added comment for clarity

os.remove(f)

print("Starting experiment creation test...\n")
try:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

There is some duplicated code here - see if you can use a helper function to centralize the try-except behavior as well as the output.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reduced redundances with added input/output functions

@rhit-fruechss rhit-fruechss left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

That all looks good - approving

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