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 tests for Scheduler job and job definition creation with input folder, refactor execution manager test #513

Merged
merged 4 commits into from
Apr 30, 2024

Conversation

andrii-i
Copy link
Collaborator

  • Add tests for Scheduler job and job definition creation with input folder
  • Refactor DefaultExecutionManager test to use db fixtures

@andrii-i andrii-i added enhancement New feature or request Testing labels Apr 30, 2024
@andrii-i andrii-i added this to the 2.6.0 milestone Apr 30, 2024
Copy link
Collaborator

@dlqqq dlqqq left a comment

Choose a reason for hiding this comment

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

Thank you for contributing this! I left some feedback below. The feedback mostly centers around using fixtures instead of hard-coded paths, so these fixtures can be easily re-used in future tests.

Comment on lines +16 to +17
TEST_ROOT_DIR = f"{HERE}/jupyter_scheduler/tests/test_root_dir"

Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. os.path.join() should be used to construct file paths, as the directory delimiter can be platform-dependent.

    • Windows uses backslashes instead of forward slashes, e.g. C:\some_dir\child_dir.
  2. When using directories in tests, we should not commit the test dirs into the source repo, as this doesn't scale well when more tests are added. Instead, you should use the tmp_path fixture provided by pytest. When used, this will create a new temporary directory (under /tmp on Linux) and return the path to that temporary directory.

    So here you would want to replace TEST_ROOT_DIR with something like:

    @pytest.fixture
    def scheduler_root_dir(tmp_path):
        return os.path.join(tmp_path, 'workspace_root')

For DB_FILE_PATH, you should use the jp_data_dir fixture provided by jupyter_server.pytest_plugin. This fixture is already under tmp_path.

NOTEBOOK_NAME = "side_effects.ipynb"
SIDE_EFECT_FILE_NAME = "output_side_effect.txt"

NOTEBOOK_DIR = Path(__file__).resolve().parent / "test_staging_dir" / "job-4"
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should have a fixture for the staging directory which uses the default staging directory name in Jupyter Scheduler:

@pytest.fixture
def staging_dir(jp_data_dir):
    return os.path.join(jp_data_dir, "scheduler_staging_area")

For the side effect test:

  1. Move output_side_effect.txt and side_effects.ipynb under a new directory containing static files used in tests, e.g. jupyter_scheduler/tests/static.
  2. Write a new staging_dir_with_side_effects fixture that copies these files to staging_dir, and returns a two-tuple of the input URI and the side effect file. So this fixture should return ('side_effects.ipynb', 'output_side_effect.txt')

job = Job(
runtime_environment_name="abc",
input_filename=NOTEBOOK_NAME,
job_id="123",
job_id=JOB_ID,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of hard-coding the job ID, you should modify this fixture to return the ID of the job record it created. This is more similar to how jobs are created by the Scheduler backend.

def test_create_job_definition_with_input_folder(jp_scheduler):
job_definition_id = jp_scheduler.create_job_definition(
CreateJobDefinition(
input_uri="job-5/import-helloworld.ipynb",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar to my previous comment, you can replace this with a input_file_with_input_dir fixture that moves import-helloworld.ipynb and a/b/helloworld.txt to scheduler_root_dir, and then return the tuple ('import-helloworld.ipynb', 'a/b/helloworld.txt').

Copy link
Collaborator

@dlqqq dlqqq left a comment

Choose a reason for hiding this comment

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

In a 1:1 conversation, @andrii-i raised a good point that my feedback falls outside the scope of this PR, and that it would more appropriate to address this feedback for all tests (not just these 2 test files) in a separate PR. I'm tracking my previous feedback in a new issue: #514.

Awesome work!

@andrii-i
Copy link
Collaborator Author

Thank you for looking into this @dlqqq

@andrii-i andrii-i merged commit 51a5ee4 into jupyter-server:main Apr 30, 2024
6 checks passed
@andrii-i andrii-i deleted the test_include_directory branch April 30, 2024 18:41
andrii-i added a commit to andrii-i/jupyter-scheduler that referenced this pull request Apr 30, 2024
…lder, refactor execution manager test (jupyter-server#513)

* use fixtures, rename job used to job-4

* test scheduler job creation and job def creation with input folder

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* remove duplciate fixture

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
andrii-i added a commit that referenced this pull request Apr 30, 2024
…lder, refactor execution manager test (#513) (#515)

* use fixtures, rename job used to job-4

* test scheduler job creation and job def creation with input folder

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* remove duplciate fixture

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants