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

define pytest logs directory #491

Merged

Conversation

hussain-jafari
Copy link

define pytest logs directory

Description

  • Category: feature
  • JIRA issue: MIC-5504

Add output-dir option when running pytest.
Create profiling directory using this output directory.
Update test_runner to write logs.

Testing

Ran test_runner.py and checked that the logs directory and profiling directory were created and populated as expected.

Copy link
Contributor

@stevebachmeier stevebachmeier left a comment

Choose a reason for hiding this comment

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

Looks great! I do think you should add descriptive IDs. I'm also concerned about a couple other small things, so be sure to address those before actually merging.

@@ -16,13 +16,31 @@
(["--dataset", "wic", "--year", "2015"]),
# (["--dataset", "wic", "--population", "USA", "--state", "RI", "--year", "2015"]),
],
ids=["1", "2", "3", "4"],
Copy link
Contributor

Choose a reason for hiding this comment

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

We'd talked at some point about automating these IDs to that (1) they are more descriptive and (2) so that they auto-adjust when you add/remove parameterizations. Maybe: ids=lambda x: "_".join([item for item in x if not item.startswith("--")]),

"--release",
"test_release.py",
"--check-max-tb=1000",
"--population",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you hard-coding population instead of parametrizing it?

assert result.returncode == 0

# log using job id
job_id = request.node.callspec.id
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is even more of a reason to descriptively handle job IDs. pytest_wic_2015.o is a lot more informative than pytest_4.o.

@@ -16,13 +16,31 @@
(["--dataset", "wic", "--year", "2015"]),
# (["--dataset", "wic", "--population", "USA", "--state", "RI", "--year", "2015"]),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these all be uncommented?

@hussain-jafari hussain-jafari merged commit 62ce877 into epic/full_scale_testing Feb 12, 2025
8 checks passed
@hussain-jafari hussain-jafari deleted the hjafari/feature/MIC-5504_pytest_logs branch February 12, 2025 19:07
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