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

Add integration tests for Script debugger #2971

Merged
merged 11 commits into from
Oct 31, 2022

Conversation

karlaspuldaro
Copy link
Member

@karlaspuldaro karlaspuldaro commented Oct 17, 2022

Closes #2897

What changes were proposed in this pull request?

Integration test cases added in scriptdebugger.ts :

  • test for debugger button to be enabled for default Python kernel
  • test for debugger button state persistence on page reload
  • test for debugger button state persistence on reopening editor tab
  • test for debugger button to be disabled for kernel without debug support

All flaky on CI environment
Follow up note: new test file name has number on purpose (for now) so it is the first to run on CI as a workaround for failures, as suggested in this comment .
Follow up issue #2995 to track investigating other test(s) file(s) that may be affecting this new one.

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

@elyra-bot
Copy link

elyra-bot bot commented Oct 17, 2022

Thanks for making a pull request to Elyra!

To try out this branch on binder, follow this link: Binder

@ptitzler ptitzler added this to the 3.13.0 milestone Oct 17, 2022
@ptitzler ptitzler added the component:test Test-related label Oct 17, 2022
@karlaspuldaro karlaspuldaro added the status:Work in Progress Development in progress. A PR tagged with this label is not review ready unless stated otherwise. label Oct 18, 2022
@karlaspuldaro
Copy link
Member Author

I'm investigating the CI failures in https://github.com/karlaspuldaro/elyra/tree/test-branch - playing with test timeouts and adding one test at a time there to see what's going on

@karlaspuldaro karlaspuldaro force-pushed the debugger-tests branch 2 times, most recently from 4b8b9b6 to 8a7dd80 Compare October 27, 2022 21:32
@karlaspuldaro
Copy link
Member Author

@kevin-bates you were right, looks like the execution order matters!
There are still 2 tests commented out for being more inconsistent, I'll introduce them back and check how things go.

@kevin-bates
Copy link
Member

Thanks for checking that @karlaspuldaro. This implies that another test (or more) is side-affecting the button's location (or something like that if I'm reading the test failures correctly).

When I've dealt with this kind of thing before, it usually means a "binary search" exercise of placing the affected test in different locations until you find the test which your test immediately fails after, but works when moved one test prior. Then, the side-effecting test can be more closely examined if the solution hasn't been discovered in the course of the search exercise. It can be a long road, but also interesting once the solution is found. Good luck. 🍀

@karlaspuldaro karlaspuldaro removed the status:Work in Progress Development in progress. A PR tagged with this label is not review ready unless stated otherwise. label Oct 28, 2022
@kevin-bates
Copy link
Member

@karlaspuldaro - since it appears we still don't know why the test location matters, could you please open an issue to revisit this as this is the kind of thing that can bite us down the road? I suspect it's one of the two "editor" tests that are causing the issue. This could be determined by moving this test immediately before those and ensuring it still passes. If there are failures, then this particular hypothesis is incorrect.

@akchinSTC akchinSTC merged commit 94da501 into elyra-ai:main Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:test Test-related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add basic debugger tests
4 participants