[Mobile Benchmark Test] Add os, job_arn, and job_conclusion to artifact #6371
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
facebook-github-bot
added
the
CLA Signed
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
yangw-dev
changed the title
| self.assertEqual(m_df.getMockClient().list_suites.call_count, 2) | ||
| self.assertEqual(m_df.getMockClient().list_tests.call_count, 4) | ||
| self.assertEqual(download_artifact_mock.call_count, 12) |
There was a problem hiding this comment.
these numbers are very precise and look like they might break the test when the code changes slightly. Can these constraints be relaxed a bit to just checking the most critical api calls and perhaps using greaterThan/lessThan type of comparisons?
The idea is to make the test more resilient to design changes in the code (so that future authors don't have to go around updating a bunch of tests) while still verifying that the core functionality desired is maintained.
| self.assertEqual(a2["name"], "test spec output") | ||
|
|
||
| @mock.patch("run_on_aws_devicefarm.download_artifact") | ||
| def test_reportProcessor_debug(self, download_artifact_mock): |
There was a problem hiding this comment.
nit: Can you please rename this test to match the "test_when_then" format the tests further down have?
That would help explain to future maintainers what exactly this test is intended to validate (which TBH isn't clear to me right now)
| self.assertEqual(m_df.getMockClient().list_suites.call_count, 2) | ||
| self.assertEqual(m_df.getMockClient().list_tests.call_count, 4) | ||
| self.assertEqual(m_s3.getMockClient().upload_file.call_count, 12) | ||
| self.assertEqual(m_s3.getMockClient().upload_file.call_count, 12) | ||
| self.assertEqual(download_artifact_mock.call_count, 12) | ||
| self.assertEqual(m_s3.getMockClient().upload_file.call_count, 12) |
There was a problem hiding this comment.
same as comment below, about making these more resilient
| run_report = self._to_run_report(report) | ||
| self.run_report = run_report |
There was a problem hiding this comment.
very minor nit, but why not:
| run_report = self._to_run_report(report) | |
| self.run_report = run_report | |
| self.run_report = self._to_run_report(report) |
(feel free to ignore if you prefer the current format)
| @@ -196,6 +200,8 @@ def parse_args() -> Any: | |||
| help="an optional file to write the list of artifacts from AWS in JSON format", | |||
| ) | |||
|
|
|||
| parser.add_argument("--debug", action="store_true") | |||
There was a problem hiding this comment.
Oh, I didn't realize until I read your comment later that debug mode doesn't upload the artifacts to S3. This is a nice feature, but I think a help description is needed here to make that clearer. You could also make some parameters like --workflow-id or --workflow-attempt optional in debug mode
There was a problem hiding this comment.
On the other hand, another option I think would be useful here is --quiet. I know that I print too many messages like INFO:root:Run mobile-job-android-13711065730-1-2025-03-07-wrRJMDcB in state RUNNING in the console log, which could be silent
| project_arn, | ||
| unique_prefix, | ||
| args.android_instrumentation_test, | ||
| args.test_spec, | ||
| ) | ||
|
|
||
| if test_to_run == {}: |
There was a problem hiding this comment.
Strictly speaking, the test suite is not a requirement because Device Farm can fuzz the app to make sure it doesn't crash. So, only the app is required. All the current use cases on CI need an test suite though, so having this check is ok. However, you could achieve this just by setting required=True in the test_group mutual exclusive group in argparse instead of doing an explicit check here
|
Does the console log from the mobile test jobs look correct https://github.com/pytorch/test-infra/actions/runs/13711065730/job/38347515619?pr=6371? You could also test this on ExecuTorch side (where it really matters) by quickly submitting a mock PR to ExecuTorch to use your branch |
|
Look like a small bug on this part https://github.com/pytorch/test-infra/actions/runs/13711065730/job/38347515619?pr=6371#step:15:81, f-string something I guess |
#6294
Details:
osin test spect printout section