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

[ATO-887] Upgrade Sanic and Sanic-testing #1057

Merged
merged 7 commits into from
Jan 8, 2024

Conversation

Tawakalt
Copy link
Contributor

@Tawakalt Tawakalt commented Jan 3, 2024

Proposed changes:

Status (please check what you already did):

  • made PR ready for code review
  • added some tests for the functionality
  • updated the documentation in the rasaHQ/rasa
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

@Tawakalt Tawakalt marked this pull request as draft January 4, 2024 23:58
@Tawakalt Tawakalt marked this pull request as ready for review January 5, 2024 00:05
@Tawakalt Tawakalt marked this pull request as draft January 5, 2024 00:06
@Tawakalt Tawakalt force-pushed the ATO-887-upgrade-sanic-and-sanic-testing branch 2 times, most recently from 073d63e to a2c0e74 Compare January 5, 2024 00:16
@Tawakalt Tawakalt force-pushed the ATO-887-upgrade-sanic-and-sanic-testing branch from 894e7e6 to 80d0324 Compare January 5, 2024 11:07
@Tawakalt Tawakalt marked this pull request as ready for review January 5, 2024 11:36
@Tawakalt Tawakalt requested review from ancalita and vcidst and removed request for ancalita January 5, 2024 11:36
pyproject.toml Outdated Show resolved Hide resolved


# ENSURE THIS IS ALWAYS THE LAST TEST FOR OTHER TESTS TO RUN
# for some reason, sys.exit is actually called here
Copy link
Member

Choose a reason for hiding this comment

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

This is actually expected given this line that gets called as a result of this test:

sys.exit(1)

I would investigate more on why this breaks the pytest command if placed anywhere else in the test module, maybe other users of Sanic have encountered this as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I did some investigation and couldn't find anything helpful.
It appeared the line was actually being executed by pytest, so any other thing afterwards isn't being run.

Not sure why it appeared with the updated Sanic version though and I couldn't find anything linking it to the issue.

Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if it's not because of the incongruent version of sanic-testing with sanic? I'd test this again once you upgrade to sanic 22.12, that's the version you used for sanic-testing.

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 was hoping that was the case too and tried it after the upgrade but it still persists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There should be a package that manages the ordering, but I'm not sure the effort is worth it for now.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah definitely not worth the effort.

Copy link
Member

Choose a reason for hiding this comment

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

I'd just replace the last line comment to say call to sys.exit() terminates pytest process or smth along those lines.

Copy link
Member

Choose a reason for hiding this comment

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

Do we know if all the tests in the suite run actually, maybe we actually need to disable this and investigate later?

Copy link
Member

Choose a reason for hiding this comment

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

Ah just checked the CI, all looks good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment updated.

@Tawakalt Tawakalt requested a review from ancalita January 8, 2024 13:51
@Tawakalt Tawakalt merged commit 0907d17 into main Jan 8, 2024
16 checks passed
@Tawakalt Tawakalt deleted the ATO-887-upgrade-sanic-and-sanic-testing branch January 8, 2024 15:09
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