Skip to content

Issue #668 - Changes for using json-automationrelevance instead of json-rev #927

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

Merged
merged 11 commits into from
Aug 30, 2021

Conversation

shubham-y
Copy link
Contributor

@shubham-y shubham-y commented May 9, 2021

Fixes #668

@marco-c marco-c linked an issue May 10, 2021 that may be closed by this pull request
@marco-c
Copy link
Collaborator

marco-c commented May 10, 2021

@shubham-y did you test the function like I described at #668 (comment)? Is it returning the same result before/after your change?

Note: there are some tests that need to be adjusted, you can look at the test logs by clicking on "Details" on the right of "Community-TC" and then opening the failing Code Coverage Backend checks: unit tests task and then clicking on "View Live Log".

@shubham-y
Copy link
Contributor Author

@marco-c yes, the function is returning the same result before and after my change.
I did look at test logs as you mentioned but struggling to find a way to pass that test.

@marco-c
Copy link
Collaborator

marco-c commented May 10, 2021

Did you manage to run the test locally?

@marco-c
Copy link
Collaborator

marco-c commented May 10, 2021

Here's the test that is failing:

def test_closest_report(mock_cache, mock_hgmo):
.

It is failing because the code it is testing is trying to load https://hg.mozilla.org/myrepo/json-automationrelevance/9921e08232584198aab1c015f3ea8b78, but we have not registered it as a mock response for the test.

The test def test_closest_report(mock_cache, mock_hgmo) is using mock_cache and mock_hgmo. mock_hgmo is the one you need to modify to fix the problem, it is registering the mock responses:

def mock_hgmo():
.
In particular, it is registering a response for json-rev (
re.compile("https://hg.mozilla.org/(.+)/json-rev/(.+)"),
), you need to change it to register a response for json-automationrelevance instead.

@shubham-y
Copy link
Contributor Author

shubham-y commented May 14, 2021

@marco-c I have finally managed to run the test locally and made a few changes in mock_hgmo as pointed out by you.
After changes now I am facing a <ExceptionInfo KeyError('changesets',) tblen=3> exception.

@marco-c
Copy link
Collaborator

marco-c commented May 14, 2021

@marco-c I have finally managed to run the test locally and made a few changes in mock_hgmo as pointed out by you.
After changes now I am facing a <ExceptionInfo KeyError('changesets',) tblen=3> exception.

This means the mock response you added doesn't have a changesets key, that you are using in the code at https://github.com/mozilla/code-coverage/pull/927/files#diff-6f7fa9ada2bb8845e9f5b8b0e5932694e9e465825627142e760f0df43b7a31e7R34.
You should make sure the mock response is the same you get when you hit the actual json-automationrelevance API.

If you can't figure it out, please push your changes so I can see them in the PR, otherwise I can't help you further.

@marco-c
Copy link
Collaborator

marco-c commented Jul 28, 2021

@shubham-y are you still interested in working on this?

@shubham-y
Copy link
Contributor Author

shubham-y commented Jul 28, 2021

@marco-c Yes, I am still interested to work on this.
I will be working on this issue this week and try and push some changes by this weekend.

@shubham-y
Copy link
Contributor Author

@marco-c I have finally managed to run the test locally and made a few changes in mock_hgmo as pointed out by you.
After changes now I am facing a <ExceptionInfo KeyError('changesets',) tblen=3> exception.

This means the mock response you added doesn't have a changesets key, that you are using in the code at https://github.com/mozilla/code-coverage/pull/927/files#diff-6f7fa9ada2bb8845e9f5b8b0e5932694e9e465825627142e760f0df43b7a31e7R34.
You should make sure the mock response is the same you get when you hit the actual json-automationrelevance API.

If you can't figure it out, please push your changes so I can see them in the PR, otherwise I can't help you further.

@marco-c I have pushed my changes, can you please help me further?

@marco-c
Copy link
Collaborator

marco-c commented Aug 30, 2021

Could you rebase on top of current master?

Copy link
Collaborator

@marco-c marco-c left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@marco-c marco-c merged commit 43beec6 into mozilla:master Aug 30, 2021
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.

Stop using json-rev everywhere
2 participants