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

Make unhide function work as expected in html reporter #4051

Merged
merged 1 commit into from
Oct 9, 2019

Conversation

pec9399
Copy link
Contributor

@pec9399 pec9399 commented Oct 7, 2019

Description

Fixed the iteration of the HTMLCollection object in the unhide function of the html reporter. Since the length of the HTMLCollection changes as the elements' class names get updated (as HTMLCollection works 'live'), it cannot be updated using an index-based for loop.

Description of the Change

Changed the index-based for loop iteration to a while loop, updating the first element while the length of the HTMLCollection is larger than 0.

Why should this be in core?

It a bug (although minor) that makes filtering by passes/failures doesn't work properly as expected.

Benefits

Filtering by passes / filters in html reporter works properly even after unhiding objects.

Possible Drawbacks

None

Applicable issues

Closes #4048

Fixed HTMLCollection iteration to make unhide function work as expected
@jsf-clabot
Copy link

jsf-clabot commented Oct 7, 2019

CLA assistant check
All committers have signed the CLA.

@juergba juergba added area: browser browser-specific status: accepting prs Mocha can use your help with this one! status: needs review a maintainer should (re-)review this pull request labels Oct 8, 2019
@juergba
Copy link
Contributor

juergba commented Oct 8, 2019

Could someone test, confirm and review this PR, please? Members, contributors, others ... ?
@Mia-jeong @HyunSangHan @Lindsay-Needs-Sleep

@Lindsay-Needs-Sleep
Copy link
Contributor

Lindsay-Needs-Sleep commented Oct 8, 2019

@juergba @pec9399
Just tried this out. Works great.

Tested on desktop chrome (77.0.3865.90) and Android (7.0) webview (77.0.3865.92).

Tried the described situations (all passes, all failures), and a mix of passes and fails. All 3 situations worked as expected.

Ran npm run test, all tests passed.

@juergba
Copy link
Contributor

juergba commented Oct 8, 2019

@Lindsay-Needs-Sleep thanks 👍

@juergba
Copy link
Contributor

juergba commented Oct 9, 2019

Since the length of the HTMLCollection changes as the elements' class names get updated (as HTMLCollection works 'live'), [...]

This sounds very weird to me with little understanding of HTML/browsers. But I got so far that:

  • document.getElementsByClassName returns an HTMLCollection which is a live list
  • document.querySelector returns a NodeList which is a static list

Is hideSuitesWithout() working correctly? As unhide() it's also using getElementsByClassName.

@pec9399
Copy link
Contributor Author

pec9399 commented Oct 9, 2019

For clarification, as you noted, document.getElementsByClassName returns an HTMLCollection which is a live list, and therefore changing the class name of an element in the HTMLCollection will remove that element from this collection right away.

In the code below, the code in line 3 will change the ith element's class name to 'suite' and consequently pop the ith element from the variable 'els' since the ith element is not included in document.getElementsByClassName('suite hidden') anymore.

  // original code

  1  var els = document.getElementsByClassName('suite hidden');
  2  for (var i = 0; i < els.length; i++) {
  3    els[i].className = els[i].className.replace('suite hidden', 'suite');
  4  }

This will inevitably change the indices of the remaining elements still in the list, making index-based loops cause unexpected behaviors when iterating over the HTMLCollection. So, I changed the code to change the class name of each element while the list is not empty rather than iterating over each index.

  // new code

  1  var els = document.getElementsByClassName('suite hidden');
  2  while (els.length > 0) {
  3    els[0].className = els[0].className.replace('suite hidden', 'suite');
  4  }

Hope this helps your understanding.

As of hideSuitesWithout(), it doesn't really have this problem. The code in line 6 will change the ith element in the variable 'suites' to 'suites hidden', but it still belongs to document.getElementsByClassName('suite'), so it will not be popped from the original HTMLCollection.

1  function hideSuitesWithout(classname) {
2   var suites = document.getElementsByClassName('suite');
3   for (var i = 0; i < suites.length; i++) {
4     var els = suites[i].getElementsByClassName(classname);
5     if (!els.length) {
6      suites[i].className += ' hidden';
7     }
8   }
9  }

@juergba juergba added type: bug a defect, confirmed by a maintainer and removed status: accepting prs Mocha can use your help with this one! status: needs review a maintainer should (re-)review this pull request labels Oct 9, 2019
@juergba juergba added this to the next milestone Oct 9, 2019
Copy link
Contributor

@juergba juergba left a comment

Choose a reason for hiding this comment

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

@pec9399 thank you for your detailed explanation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: browser browser-specific landed-on-v6.2.x cherry-picked on v6.2.x branch from master semver-patch implementation requires increase of "patch" version number; "bug fixes" type: bug a defect, confirmed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Html Reporter Filtering Issue when number of passes/failures is 0
5 participants