Skip to content

Conversation

@alanag13
Copy link
Contributor

@alanag13 alanag13 commented Feb 4, 2021

The nightly build was failing due to some tests that only worked when run in a certain order. Going through all tests and running them individually exposed which tests needed adjusting.

@github-actions
Copy link

github-actions bot commented Feb 4, 2021

CLA Assistant Lite bot All contributors have signed the CLA ✍️


def test_show_when_py42_raises_exception_prints_error_message(runner, cli_state, error):
cli_state.sdk.cases.file_events.get_all.side_effect = Py42NotFoundError(error)
def test_show_when_py42_raises_exception_prints_error_message(
Copy link
Contributor

Choose a reason for hiding this comment

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

For CASE_DETAILS, can we make that the actual response for getting a case? And maybe rename it to CASE_DETAILS_RESPONSE_TEXT or something similar

@antazoey
Copy link
Contributor

antazoey commented Feb 4, 2021

can we make the text for the custom_error fixture JSON text?

obj=cli_state,
)
cli_state.sdk.cases.file_events.add.assert_called_once_with(1, "1")

Copy link
Contributor

@antazoey antazoey Feb 4, 2021

Choose a reason for hiding this comment

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

On line 229, it seems to me like the wrong method is being mocked ... I don't understand what is going on there. get_all does not get called in show, as far as I can tell.

shouldnt we be mocking cases.get() instead?

Copy link
Contributor

@kiran-chaudhary kiran-chaudhary Feb 5, 2021

Choose a reason for hiding this comment

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

the test method at line 229, test_show_prints_expected_data_with_include_file_events_option , mocks file-events get_all, so its fine. When --include-file-events is passed we want all the events listed against the case.

@antazoey
Copy link
Contributor

antazoey commented Feb 4, 2021

Consider it approved on my end, but I found some other oddities and ways that would make the tests less likely to break from py42 changes.

Copy link
Contributor

@kiran-chaudhary kiran-chaudhary left a comment

Choose a reason for hiding this comment

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

Changes LGTM.

Honestly, Looking at the refactor, I am not able to tell what issue has been resolved, some more detail in the description, a link to the nightly build execution, would have helped to understand the scenario.

Having said that, I will try to find the nightly build execution, that shouldn't be hard assuming it should be in last few executions somewhere.

@kiran-chaudhary
Copy link
Contributor

kiran-chaudhary commented Feb 5, 2021

Changes LGTM.

Honestly, Looking at the refactor, I am not able to tell what issue has been resolved, some more detail in the description, a link to the nightly build execution, would have helped to understand the scenario.

Having said that, I will try to find the nightly build execution, that shouldn't be hard assuming it should be in last few executions somewhere.

Please ignore this. I started my day with reviews and show the slack conversation later, it did help to understand the investigation part.

@alanag13 alanag13 merged commit 9d75cc0 into master Feb 5, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Feb 5, 2021
@timabrmsn timabrmsn deleted the INTEG-1498-fix-nightly-builds branch August 27, 2021 18:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants