| sean-k-mooney | fungi: i made some concreate progress on the code review job over the weekend and got it to review the change to defien itself https://zuul.teim.app/t/main/builds?job_name=openstack-ai-code-review&project=SeanMooney/openstack-ai-style-guide, most of the failrue are related to an apprently know api bug that happens when using the combiniation of lightllm as a proxy with opencode | 11:00 |
|---|---|---|
| sean-k-mooney | and z.ai when z.ai is under load. apprently the issue is specific to glm and the openai compatible endpoint os using the antropic endpoint with claude code _shoud_ mitigate that but i have not had time to test that. | 11:00 |
| sean-k-mooney | before sharign this too widely im probaly going to reband this ci to teim-ci instead of sean-mooney-ci as i have been told in the past that my ci account sometimes confuses peopel about which account to add | 11:01 |
| sean-k-mooney | ill proably also drop the "openstack" out of the job name and call it teim-ai-code-review instead | 11:01 |
| sean-k-mooney | or just ai-code-review. | 11:01 |
| sean-k-mooney | this is an example of the review report that is generated https://zuul.teim.app/t/main/build/92ad2b0760d74b9c99c98525f9a03377/log/code-review/review-report.md | 11:04 |
| sean-k-mooney | i am probably going to enhance it to actully produce a nice html report as well later but its markdown for now. | 11:05 |
| chandankumar | Hello #openstack-tc, Under watcher project, watcher-dashboard is the horizon plugin. The watcher-dashboard has zero unit/functional/integration test coverage. | 12:20 |
| chandankumar | We attended the testing session of Horizon PTG< https://etherpad.opendev.org/p/horizon-gazpacho-ptg#L116 >. They have developed reusable tests and fixtures based on selenium and pytest < https://github.com/openstack/horizon/tree/master/openstack_dashboard/test/selenium > with zuul job and tox targets. Manila-ui (horizon plugin for manila) is already reusing these tests and fixtures to add pytest based integration | 12:20 |
| chandankumar | tests in their repo < https://github.com/openstack/manila-ui/tree/master/manila_ui/tests/selenium >. | 12:20 |
| chandankumar | Based on this session, we talked about adding unit/integration tests in watcher-dashboard repo. Based on https://github.com/openstack/governance/blob/master/reference/pti/python.rst#python-test-running, We need to write tests based on Python stdlib unittest module so that test runner like stestr should run it. | 12:20 |
| chandankumar | Since Horizon integration test is based on pytest, manila-ui is already using it. Other horizon plugin might adapt in future. | 12:21 |
| chandankumar | Pytest is also used by skyline-apiserver < https://opendev.org/openstack/skyline-apiserver/src/branch/master/tox.ini#L75 >, rally, rally-openstack projects. projects in airship and starlingx namespace are also using it < https://codesearch.openstack.org/?q=pytest&i=nope&literal=nope&files=&excludeFiles=&repos= > | 12:21 |
| chandankumar | in order to reuse in watcher-dashboard project, can we modify the Python PTI document to include pytest tests there also? otherwise we need to reimplement those tests | 12:21 |
| chandankumar | based on unittest module. | 12:21 |
| sean-k-mooney | chandankumar: that not actuly entirly ture the watcher-dashboard has existing unit and seleium tests but the latter was likely broken by the rewrite of horizong test to use pytest | 13:49 |
| sean-k-mooney | this is the existing test coverage https://github.com/openstack/watcher-dashboard/tree/master/watcher_dashboard/test and here are or existing selenium tests https://github.com/openstack/watcher-dashboard/blob/master/watcher_dashboard/test/integration_tests/pages/admin/optimization/auditspage.py | 13:51 |
| sean-k-mooney | so to a limit degree we have been allowing a lower bar for test coverage then we shoudl enfoce in https://github.com/openstack/watcher-dashboard but its not true that there is 0 coverage however the selinium test that do exist dont run in any ci job that im aware of | 13:53 |
| sean-k-mooney | and as i staid they are now likely broken since they were written before the pytest rewrite in horizon | 13:53 |
| sean-k-mooney | as show by our test-requirements.txt we expect our test to use unittests and the related package like testtools https://github.com/openstack/watcher-dashboard/blob/master/test-requirements.txt | 13:54 |
| sean-k-mooney | because of historical bagage teh selenium tests are un vai https://github.com/openstack/watcher-dashboard/blob/master/run_tests.sh and or django manage rather then tox because historicly horizon plugins did not really follow the pti | 13:56 |
| sean-k-mooney | and instead followed the django convetions | 13:57 |
| sean-k-mooney | it is however entilry possibel to use stster and tox to run django based tests https://opendev.org/openstack/grian-ui/src/branch/master/tox.ini#L6-L83 grian-ui is still effectivly an empty project but ensureing we coudl do that https://opendev.org/openstack/grian-ui/src/branch/master/tests/grian_ui_tests/unit/test_main.py | 13:59 |
| sean-k-mooney | https://opendev.org/openstack/grian-ui/src/branch/master/tests/grian_ui_tests/unit/base.py was one of the first thing i did | 13:59 |
| chandankumar | sean-k-mooney: thank you for correcting about the test coverage. I was only running client tests via tox and rest of tests were ignored. I was also working on dropping those integration tests based on horizon https://review.opendev.org/c/openstack/watcher-dashboard/+/964775 that's why added zero test. | 14:02 |
| chandankumar | The watcher-dashboard tests are broken due to horizon test rewrite. | 14:03 |
| chandankumar | By using unittest module based grian-ui example, yes it is possible to write the test. | 14:04 |
| sean-k-mooney | ya so in either case we willl effecitvly be starting from scratch which given the small amount of existing test is fine | 14:04 |
| sean-k-mooney | chandankumar: i mentioned this in the ptg but django itself also uses unittest not pytest for all of its onw testing and its documenation of how to write tests | 14:05 |
| sean-k-mooney | so giran-ui was going ot use unittest both to align to openstacks pti and djangos default testing ecosystem | 14:05 |
| chandankumar | If we donot use pytest tests from horizon, we will be ending with developing our own tests. We may not get feedback from Horizon. | 14:07 |
| sean-k-mooney | yep which if we dont agree to update the pti would be the correct course fo action | 14:08 |
| chandankumar | few other projects are also using pytest in openstack. | 14:08 |
| sean-k-mooney | correct becuase tecnially they are not allowed to use it | 14:08 |
| sean-k-mooney | that can use it as a test runner but not in test writing | 14:08 |
| chandankumar | yup, but let's see what TC says on this. | 14:10 |
| sean-k-mooney | just to be clear even if the tc is allows it that does not mean we will adopt it we may but i want to be convicned why we should use pytest for watcher-dashbaord and unittest for watcher | 14:12 |
| chandankumar | sean-k-mooney: I am fine going with unittest, only downside I am seeing I am not sure how much we will get feedback from Horizon team on UI tests. | 14:13 |
| sean-k-mooney | the pti conformance is a hard blocker for me the use of pytest in test code is also a concern in general but lets see what the tc thinks | 14:14 |
| sean-k-mooney | chandankumar: i dont really expect them to provide feedback in general it woudl be nice if they did but i dont assume they will | 14:14 |
| chandankumar | ok | 14:16 |
| sean-k-mooney | my assumtion is the majoriyt of the review and matiantance of watcher-dashboard will be done by the watcher team as it has historically been so i want to ensure we have a full understanding of the testing framwork and famialrty with it | 14:17 |
| chandankumar | yes totally agree! | 14:23 |
| sean-k-mooney | note that we shoudl ideally ahve a spec for this on the watcher side, we coudl use the one i started but we need to enhance it with a dicussion fo the design an impleaiton or ideally we woudl write a seperate one just for the integration tests. | 14:26 |
| chandankumar | seperate spec for integration tests sounds good. | 14:32 |
| chandankumar | gouthamr: hello, Can we add above topic ^^ to get TC opinion on usage of pytest in OpenStack for tomorrow tc meeting. | 14:40 |
| chandankumar | Thank you! | 14:40 |
| chandankumar | the tc meeting is quite late for me. I will check the response tomorrow. | 14:41 |
| chandankumar | sean-k-mooney: thank you for all the input! | 14:41 |
| fungi | sean-k-mooney: i'd probably make it slightly more specific than ai-code-review since job definitions are global and it's possible we'll end up with more than one | 15:09 |
| sean-k-mooney | ya thats a good point so teim-code-review woudl be better for my third party ci job | 15:10 |
| sean-k-mooney | although depending on if you import jobs form the repo the woudl defien the upstream versoin the collision may or may not be a problem | 15:10 |
| sean-k-mooney | teim is the anglasised spelling of téim the irish verb "to go" teim.app is my newish shorter domain for hosting thigns hence why im moving to using that as the prefix for my things like this | 15:12 |
| fungi | oh, yeah, sorry i was conflating the gerrit account name and upstream vs third-party runners. if the gerrit username is unique enough then having a generic job name isn't super problematic as long as you don't end up with any collisions from importing other repos | 15:13 |
| sean-k-mooney | ya so my displayname for my ci accound it sean-mooney-ci | 15:13 |
| sean-k-mooney | i was going t rename that to teim-ci | 15:13 |
| sean-k-mooney | again avoiding the é because while that is mostly safe acents sometimes break things | 15:14 |
| sean-k-mooney | its why i dont actully spell my name seán online | 15:14 |
| sean-k-mooney | fungi: this is my actual ci account https://review.opendev.org/dashboard/28006 | 15:17 |
| sean-k-mooney | im assuming if i change the displayname it will change the name shown for the comment but its not really a big deal if it does not | 15:17 |
| fungi | yeah, that should work if your plan is to change the display name. gerrit doesn't support username changes, so if you need a different actual username then that would involve creating a new account | 15:18 |
| sean-k-mooney | yep. again im just trying to aovid confution by making it clear what is done via my ci vs me | 15:19 |
| sean-k-mooney | i coudl create a new account if it end up being an issue | 15:20 |
| sean-k-mooney | but for now since its woring with my old ci account ill just use that | 15:20 |
| fungi | as a followup to the "openinfra universe" discussion at the ptg, wes made a slight change to allow adding projects without logos, in case there are any people want added that don't have one | 19:28 |
Generated by irclog2html.py 4.0.0 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!