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 assertion to check if the order of test paths is correct #856

Merged
merged 2 commits into from
May 20, 2024

Conversation

ono-max
Copy link
Contributor

@ono-max ono-max commented May 13, 2024

Currently, we do not verify the order of JSON at https://github.com/launchableinc/cli/blob/main/tests/cli_test_case.py#L221 because we evaluate JSON in an order-agnostic manner. However, we aim to ensure the order of test paths is accurate. assert_json_orderless_equal disregards the order of items intentionally. Consequently, I have introduced the assert_test_path_orderly_equal method to address this requirement, instead of modifying assert_json_orderless_equal.

@kohsuke

I think you designed these methods in #91. Can you take a look at this PR?

@ono-max ono-max requested a review from kohsuke May 13, 2024 06:02
@@ -294,3 +295,15 @@ def tree_sorted(obj):
return obj

self.assertEqual(tree_sorted(a), tree_sorted(b))

def assert_test_path_orderly_equal(self, a, b):
Copy link
Member

Choose a reason for hiding this comment

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

Let's define the contract of this method. What exactly does this method assert?

I ask because I was expecting a and b are symmetric, but L308-309 doesn't work that way. For exampe, if a is empty list, this assertion passes no matter what b is.

I'm almost tempted to just test a['events']['testPath']==b['events']['testPath']; Is that not what we want to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm almost tempted to just test a['events']['testPath']==b['events']['testPath']; Is that not what we want to do?

I initially planed to do, but it's not possible because the value associated with the 'events' key is in random order.

Copy link
Contributor Author

@ono-max ono-max May 20, 2024

Choose a reason for hiding this comment

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

https://github.com/launchableinc/cli/pull/856/files#diff-b0c1be9f593a5643f1de3766e049337b1abde38f8fe1e6f244d09125bf8c03e0R191-R242 is an example to describe the situation. I'd like to check only the order of elements in the value of testPath key.

@@ -20,27 +20,6 @@ def test_record_test_minitest(self):
self.assert_success(result)
self.assert_record_tests_payload('record_test_result.json')

# For testing https://github.com/launchableinc/cli/pull/846/files#r1591707246.
Copy link
Member

Choose a reason for hiding this comment

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

Is this removal intentional?

Copy link
Contributor Author

@ono-max ono-max May 20, 2024

Choose a reason for hiding this comment

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

Yes, it is. I've added this test for testing the order as a temporary solution. However, by adding this assertion, this test method will be meaningless.

Copy link

sonarcloud bot commented May 20, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@ono-max ono-max requested a review from kohsuke May 20, 2024 07:32
@kohsuke
Copy link
Member

kohsuke commented May 20, 2024

Thanks! There's some loose ends when duplicates are present but now the intent is clar, so that's OK for now.

@ono-max ono-max merged commit 0ba002d into main May 20, 2024
15 checks passed
@ono-max ono-max deleted the check-order branch May 20, 2024 23:18
@github-actions github-actions bot mentioned this pull request May 20, 2024
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.

None yet

2 participants