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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix intermittent test failure re: destroyed tree-view #1288

Merged
merged 3 commits into from Nov 27, 2018

Conversation

Projects
None yet
2 participants
@jasonrudolph
Member

jasonrudolph commented Nov 21, 2018

Description of the Change

We've been seeing this assertion fail intermittently on AppVeyor recently:

expect(treeView2.element.outerHTML).toBe(treeViewHTML)

Example failing build: https://ci.appveyor.com/project/Atom/tree-view/builds/20338242/job/v7t0rvbkmv8ih0mq

What specifically is failing?

We can see the output of the failing assertion here in the build log here. Let's look at a diff between the expected value and the actual value in that assertion:

@@ -1,6 +1,6 @@
 <div class="tool-panel tree-view" tabindex="-1">
   <ol class="tree-view-root full-menu list-tree has-collapsable-children focusable-panel" style="">
-    <li is="tree-view-directory" class="directory entry list-nested-item project-root status-added expanded selected">
+    <li is="tree-view-directory" class="directory entry list-nested-item project-root expanded selected">
       <div class="header list-item project-root-header"><span class="name icon icon-file-directory" data-path="C:\projects\tree-view\spec\fixtures\root-dir1" data-name="root-dir1" title="root-dir1">root-dir1</span></div>
       <ol class="entries list-tree">
         <li is="tree-view-directory" class="directory entry list-nested-item collapsed" draggable="true">
@@ -19,14 +19,15 @@
         <li is="tree-view-file" draggable="true" class="file entry list-item"><span class="name icon icon-file-text" title="tree-view.txt" data-name="tree-view.txt" data-path="C:\projects\tree-view\spec\fixtures\root-dir1\tree-view.txt">tree-view.txt</span></li>
       </ol>
     </li>
-    <li is="tree-view-directory" class="directory entry list-nested-item project-root status-added expanded">
+    <li is="tree-view-directory" class="directory entry list-nested-item project-root expanded">
       <div class="header list-item project-root-header"><span class="name icon icon-file-directory" data-path="C:\projects\tree-view\spec\fixtures\root-dir2" data-name="root-dir2" title="root-dir2">root-dir2</span></div>
       <ol class="entries list-tree">

The only difference is that the actual content is missing the status-added class.

Why is the the status-added class missing?

From what I can tell, the status-* classes get added asynchronously as a side effect of creating or altering a Directory. Interestingly, we have a set of tests that cover the handling of these status classes. For example:

describe "when a directory is new", ->
it "adds a custom style", ->
expect(treeView.element.querySelector('.project-root .directory.status-added').header).toHaveText('dir2')

And even more interestingly, those tests explicitly configure the tree view to use synchronous file system calls before asserting the state of the status-* classes:

treeView.useSyncFS = true

Given that the failure we're seeing has to do with a missing status-added class, it seems like the intermittent test failure is the result of a nondeterministic behavior due to asynchronous file system calls.

Proposed solution

1b731e3...bcbf991 explored the possibility of updating the test to use synchronous file system calls, but that approach is insufficient/problematic for the reasons described in #1288 (comment).

Instead, as described in #1288 (comment), this pull request updates the test to be less fragile while still exercising the core functionality under test. Instead of asserting a character-by-character match of the original tree-view HTML and the re-created tree-view HTML (which is dependent on asynchronous Git status operations), the updated implementation ensures that the number of entries in the original tree-view matches the number of entries in the re-created tree-view. To me, this feels like it strikes a nice balance:

  • It should avoid the intermittent failures that we've been seeing in this test, and ...
  • It should still fail if we make a change that prevents the tree-view from being re-created after it gets destroyed (which is the essence of the test)

Alternate Designs

See #1288 (comment), which discusses an alternative based on the use of useSyncFS and updateRoots.

Benefits

Assuming this works, the test will consistently pass 馃檹

Possible Drawbacks

None that I'm aware of

Applicable Issues

Refs #789 (which introduced the use of synchronous file system calls for the set of tests referenced above)

Closes #1276

@jasonrudolph jasonrudolph requested a review from 50Wliu Nov 21, 2018

@jasonrudolph

This comment has been minimized.

Member

jasonrudolph commented Nov 21, 2018

Well, this change didn't resolve the problem after all. 馃檧Even with this change in place, we still sometimes see the failure:

https://ci.appveyor.com/project/Atom/tree-view/builds/20474724/job/i99d26hupv8oh6lr#L124

Back to the drawing board. 馃

@jasonrudolph jasonrudolph removed the request for review from 50Wliu Nov 21, 2018

@50Wliu

This comment has been minimized.

Member

50Wliu commented Nov 21, 2018

I'm thinking you might need two useSyncFS calls: one after this line, where we re-assign treeView, and one after we grab the new tree-view.

@jasonrudolph

This comment has been minimized.

Member

jasonrudolph commented Nov 22, 2018

Thanks, @50Wliu. 馃檱

I was hoping that suggestion might do the trick, but even with that change, we still sometimes see the failure:

https://ci.appveyor.com/project/Atom/tree-view/builds/20476994/job/sfejnd0vu9ajhd6h#L134

I think the key difference is that the other tests call updateRoots right after setting useSyncFS to true:

treeView.useSyncFS = true
treeView.updateRoots()

updateRoots creates brand new Directory instances, and those instances get created with useSyncFS set to true.

We could update this test to also call updateRoots, and that would probably resolve the intermittent failure, but I want to see if there's another option. The essence of this test doesn't involve synchronous file system calls or Git statuses, so I'd love to find a way to avoid cluttering the test with those concerns. I'm gonna sleep on it and see if any alternatives come to mind. 馃槾馃

@jasonrudolph

This comment has been minimized.

Member

jasonrudolph commented Nov 26, 2018

The essence of this test doesn't involve synchronous file system calls or Git statuses, so I'd love to find a way to avoid cluttering the test with those concerns.

I've updated the implementation to be less fragile while still exercising the core functionality under test. Instead of asserting a character-by-character match of the original tree-view HTML and the re-created tree-view HTML (which is dependent on asynchronous Git status operations), the updated implementation ensures that the number of entries in the original tree-view matches the number of entries in the re-created tree-view. To me, this feels like it strikes a nice balance:

  • It should avoid the intermittent failures that we've been seeing in this test, and ...
  • It should still fail if we make a change that prevents the tree-view from being re-created after it gets destroyed (which is the essence of the test)

I ran the build five times on AppVeyor, and it passed every time...

馃槄

@jasonrudolph jasonrudolph requested a review from 50Wliu Nov 26, 2018

@50Wliu

I suppose my only concern with this approach is that the entries are added correctly - that is, they have the correct nesting, directories and files are separated, etc. On the other hand, it looks like that's tested in

it('restores the expanded directories and selected files', () => {
so we should be ok.

@jasonrudolph

This comment has been minimized.

Member

jasonrudolph commented Nov 27, 2018

Thanks for the review, @50Wliu. 馃檱

@jasonrudolph

This comment has been minimized.

Member

jasonrudolph commented Nov 27, 2018

FYI: I've updated the pull request body to reflect the updated implementation.

@jasonrudolph jasonrudolph merged commit d547aef into master Nov 27, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jasonrudolph jasonrudolph deleted the fix-flaky-test-about-recreating-view branch Nov 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment