-
Notifications
You must be signed in to change notification settings - Fork 496
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
NAS-130937 / 25.04 / Remove REST from test_290_mail.py #14422
Conversation
tests/api2/test_mail.py
Outdated
"smtp": True, | ||
"user": "[email protected]" | ||
} | ||
call("mail.update", payload) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be done with context call or try so that we maintain the same configuration exiting the test as we had entering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests we have now do not do this, but I can add it. The problem is if the test fails, restoring the state of the configuration will probably also fail since it would use the same endpoints (mail.config
and mail.update
) that the test uses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many of the old tests did not attempt to restore state. Many (most?) of the updated and new tests do proper state restore. Yes, if the endpoint is failing, then restoring the state is mute because you weren't able to effect any change in the first place.
It's not critical, but good practice for a CI test environment.
I'll approve and let you decide which way to go.
Ah, you did add restore! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it, will keep in mind as I'm going through the "rest" of the tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approve the pun. 😄
This PR has been merged and conversations have been locked. |
Test: http://jenkins.eng.ixsystems.net:8080/job/tests/job/api_tests/436/