Write functional javascript test for block visibility vertical tabs

I have reviewed the related issue and added test coverage to address the mentioned points. I have attempted to test all the scenarios outlined in the issue. Could you please review this and let me know if there’s anything that needs to be updated or improved?

@shalina_jha I think test coverage should be failing. Since the fix is in the related issue's (3493182) MR/ fork. So this test is running against the existing code which will contain the bug.

@shalini_jha I think adding asserts to the test to check that if there are > 1 vertical tabs that the text is repeated x times where x is the number of tabs: that will be fine.

@shalini_jha I have made slight changes making the code a bit simpler and more specific. Ready for review.

@oily Thanks for your review . Based on the previous issue, i have tried to show the test coverage for vertical tab summary text is appended to all vertical tabs label. Additionally, I have implemented checks to confirm that clicking on any tab opens the corresponding form. we can wait for review whether we need to update this.

@shalina_jha Yes i noticed that you have stuck closely to the requirements of the parent issue. Looks good. I am not sure if using ->find('css', [some class or id]) loops through each of the tabs. If so then maybe it finds if any of the tabs have the 'bad text' repeated in the title. It might also just find the first element that matches and apply the assert to that? But then ignore the other tabs. If it checks all the matching elements and applies the assert to all of them then that is not good in my opinion.

If for any reason the 'bad text' appeared in all except one of the tabs then that is something we would want the test to find and fail on.

A way to verify that this tests the specific bug in 🐛 Block visibility settings have summary duplicated in the title Active would be to add an additional branch here, revert the commit from that issue, and open a draft MR - then we should be able to see the new test coverage fail.

@catch Thank you. That sound like the way ahead..

Thanks for working on this. I left some comments in the MR.

@quietone Thank you for the code suggestions.

I applied the suggestion of @catch at #12. The new branch and MR are visible above. Not yet sure why but the FunctionalJavascript tests keep passing in the pipeline. But locally in DDEV the test fails and the ouput seems correct See screenshot →

There is one failing FJ test in PerformanceTest.php. Seems unrelated to this issue. It is the result of a rebase I did.

Changing to Needs Review.

Pipeline all green.

While i am checking the feedback changes from initial work, observe the failure in MR 10635 , I have check this and its a random failure and re run this. now its fixed. Thanks @oily for updating the feedback. leaving this as NR.

Thank you, @shalini_jha, glad to help.

@oily , As i have already debug and worked on the test coverage and moved this ticket to NR for the feedback what need to be change or update, Please Left this issue for 2nd review.i want to try 2nd review.

The branch at https://git.drupalcode.org/project/drupal/-/merge_requests/10635/diffs looks like the branch has been rolled back to before the commit, then the tests here added. This will pass against HEAD because the test process merges that branch into HEAD, which means it's just adding the test without reverting the fix.

To get the MR to fail, instead you'd need to do git revert 89a009ac - this will make a new commit reversing the fix, then that will get get applied when the pipeline runs.

I have Tried to follow the #25 , same way first i have rebase the MR(10635) and then applied this "git revert 89a009ac" and pushed the changes to the https://git.drupalcode.org/project/drupal/-/merge_requests/10635/diffs, same mentioned this has generated the new commit to revert the fix for the issue. However, the pipeline is still passing here, but when I run the test locally, it is failing in this branch.

For debugging tried to Run pipeline again but it still passing , the reverted code is there in MR , but pipeline not showing the failing test , but locally it is working.

@catch Thank you for the advice.

@shalini_jha Yes, @catch has advised on how to proceed. It wont work until we follow #25.

@oily I have already followed the step mentioned by @catch in #25, You can check #26 comment, pipeline is still passing.

Ok, maybe @catch will advise or can reach out on Slack

Hi @catch,
Could you please provide some guidance? I have followed the steps mentioned by you in #25 exactly as outlined. However, the functional JavaScript test is still passing, which suggests that it might still be working against the HEAD. I’m unsure why this test is passing. Any insights would be greatly appreciated.Thank you!

There are 2 open MRs when there should only be 1.

Issue summary is also incomplete

oily → changed the visibility of the branch 3493914-revert-to-just-before-parent-issue-commit-for-tests to hidden.

I used my testing issue to revert commit 89a009ac124657011ecaa0830a2950da48fa37ad and then apply the diff from this issue. I get the same results as @shalini_jha. That is, BlockAddTest fails locally but passes on GitLab. And worse, the GitLab artifact does not contain the browser output from that test, while it does for other tests.

Asked in Slack about this and learned about 🐛 Functional Javascript test false postive and missing browser output Active

@smustgrave the second MR open here is to try to demonstrate that the tests fail without a bug fix from a previous issue, unfortunately it looks like functional js tests aren't failing properly on gitlab, but it's here for a good reason and not a reason to mark the issue needs work.

fwiw the test-only MR looks right and I think we're just running into the gitlab bug, good that @quietone was able to see the failure locally (and find out about the bug in general, that's very confusing).

Issue summary looks fine now so moving to RTBC.

Sorry but we’ve moved other issues with multiple MRs and incomplete issue summaries to NW that’s why I did

Thank you @smustgrave, @quietone and @catch for moving this along.

Thanks you @shalini_jha for building the test.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Fixed Bot reported error, so i am moving this NR instead of RTBC.

Restoring status.

Committed/pushed to 11.x, thanks!

catch → closed merge request !10619

Automatically closed - issue fixed for 2 weeks with no activity.