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

fix(camelContext): expand tree for routes, endpoints & components #695

Merged
merged 1 commit into from
Dec 6, 2023

Conversation

kahboom
Copy link
Member

@kahboom kahboom commented Dec 1, 2023

Description

This PR expands the Camel context tree by one level, leaving MBeans collapsed. Resolves #665 .

Camel Context Tree

@tadayosi - Apologies for the delay on this issue. There were no lint issues with the switch statement. Some questions:

  1. Is it a problem that I've done child.defaultExpanded = true? This will expand all root children by one (in this case, SampleCamel. Or is there a particular property you want me to do a conditional check for?
  2. I couldn't see an easy way to test this implementation with unit tests or E2E tests. Please let me know if there's anything you'd like for me to do in this regard for this PR.

Copy link

github-actions bot commented Dec 1, 2023

Test Results

    8 files  ±0      8 suites  ±0   41m 0s ⏱️ +6s
  59 tests ±0    58 ✔️ ±0  1 💤 ±0  0 ±0 
468 runs  ±0  460 ✔️ ±0  8 💤 ±0  0 ±0 

Results for commit 3869754. ± Comparison against base commit 141d99f.

♻️ This comment has been updated with latest results.

@hawtio-ci
Copy link

hawtio-ci bot commented Dec 1, 2023

Test results

Run attempt: 896
Detailed summary

NAME TESTS PASSED ✅ SKIPPED 💤 FAILED ❌ ERRORS 🚫 TIME 🕖
results-quarkus-node(18)-java(11)-firefox 59 58 1 0 0 321.844
results-quarkus-node(18)-java(17)-firefox 59 58 1 0 0 311.578
results-quarkus-node(20)-java(11)-firefox 59 58 1 0 0 308.205
results-quarkus-node(20)-java(17)-firefox 59 58 1 0 0 318.746
results-springboot-node(18)-java(11)-firefox 58 57 1 0 0 328.555
results-springboot-node(18)-java(17)-firefox 58 57 1 0 0 292.282
results-springboot-node(20)-java(11)-firefox 58 57 1 0 0 286.166
results-springboot-node(20)-java(17)-firefox 58 57 1 0 0 293.233

@phantomjinx
Copy link
Member

@kahboom

I couldn't see an easy way to test this implementation with unit tests or E2E tests. Please let me know if there's anything you'd like for me to do in this regard for this PR.

You can probably add a render test (reacting-testing-library) to CamelTreeView.test.tsx. Once the context renders you should be able to check that the nodes have been expanded.

Copy link
Member

@tadayosi tadayosi left a comment

Choose a reason for hiding this comment

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

Thanks, I really like the simplicity of the fix.

There's only one minor comment that should be easy to address.

packages/hawtio/src/plugins/camel/context.ts Outdated Show resolved Hide resolved
@tadayosi
Copy link
Member

tadayosi commented Dec 4, 2023

  • Is it a problem that I've done child.defaultExpanded = true? This will expand all root children by one (in this case, SampleCamel. Or is there a particular property you want me to do a conditional check for?

The requirement natually demands the camel context node to be expanded, so it should be no problem. As commented above, we should just move the forEach logic out of the path expansion logic.

By the way, Camel v4 enforces that we only have one Camel context, so we can assume that the root only has one child. However, we still contain children as list, so considering the requirement we shouldn't need to expand (virtual) second or third Camel context nodes. So the logic can focus on the first child of the root node (i.e. Camel Contexts).

  • I couldn't see an easy way to test this implementation with unit tests or E2E tests. Please let me know if there's anything you'd like for me to do in this regard for this PR.

Yes, we struggle to have better component test suite in this project. We can tackle this issue separately. For the time being, we just need to pass all the existing E2E tests (based on Cucumber), which unfortunately all failed now.

@tadayosi
Copy link
Member

tadayosi commented Dec 4, 2023

@mmuzikar @jsolovjo Do you have any ideas why E2E fails here? The fix only changes the way how the Camel tree is expanded by default. I'd suspect the E2E assumes that the tree is all folded and otherwise it fails, is it the case?

@jsolovjo
Copy link

jsolovjo commented Dec 4, 2023

@mmuzikar @jsolovjo Do you have any ideas why E2E fails here? The fix only changes the way how the Camel tree is expanded by default. I'd suspect the E2E assumes that the tree is all folded and otherwise it fails, is it the case?

Hi @tadayosi I will have a look at this

UPD: from a quick look I see that the test tries to create an endpoint `mock://bar' from Data and it seems it didn't get created and therefore there are failures of the tests which rely on that endpoint

I will continue checking it and try to fix it

@jsolovjo
Copy link

jsolovjo commented Dec 4, 2023

@mmuzikar @jsolovjo Do you have any ideas why E2E fails here? The fix only changes the way how the Camel tree is expanded by default. I'd suspect the E2E assumes that the tree is all folded and otherwise it fails, is it the case?

Hi @tadayosi I will have a look at this

UPD: from a quick look I see that the test tries to create an endpoint `mock://bar' from Data and it seems it didn't get created and therefore there are failures of the tests which rely on that endpoint

I will continue checking it and try to fix it

@tadayosi I found the issue, let's say it's a bug in our tests.

We use one unified method used for clicking on buttons (we pass a parameter which usually a button's text and find the button by its type and text), in this scenario we have to add an endpoint from Data and then in one step test scenario we have to choose mock from the drop-down list and the items in this list are buttons. At the same time, the Camel tree elements are also buttons and if you look at the screenshot, the test finds two buttons with mock name and clicks on the first found and it's from the Camel tree, and therefore the tests fail. I will try to fix the test.

Screenshot 2023-12-04 at 13 52 38

@kahboom
Copy link
Member Author

kahboom commented Dec 5, 2023

@tadayosi - Thanks for your help, I've made the changes.

@phantomjinx - For some reason running yarn test:all fails for me locally despite passing on CI/CD. Any idea why? This is without any changes or added tests, and the same occurs on main.

chore(lint): fix linting error

fix: move logic out of forEach of rootNode

fix: lint
@tadayosi
Copy link
Member

tadayosi commented Dec 6, 2023

@kahboom Thanks. I'll take care of the lint error this time. Next time, please remember to run yarn format:check as well.

@tadayosi tadayosi merged commit 499e118 into hawtio:main Dec 6, 2023
12 of 13 checks passed
@kahboom
Copy link
Member Author

kahboom commented Dec 8, 2023

Ah apologies @tadayosi I thought I only needed to run yarn lint --fix.

@kahboom kahboom deleted the camel-tree-view branch December 8, 2023 14:08
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.

Camel - Tree view should be expanded by default
4 participants