Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Fix framework grouping collapsing #3434

Merged
merged 1 commit into from Jul 25, 2017

Conversation

jasonLaster
Copy link
Contributor

@jasonLaster jasonLaster commented Jul 24, 2017

Associated Issue: #

Summary of Changes

  • fixes collapsing for both frames that are not grouped

@jasonLaster
Copy link
Contributor Author

I'd like to add a unit test here, but struggled with it

@codecov
Copy link

codecov bot commented Jul 24, 2017

Codecov Report

Merging #3434 into next will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##            next    #3434      +/-   ##
=========================================
+ Coverage   49.8%   49.82%   +0.02%     
=========================================
  Files        109      109              
  Lines       4516     4516              
  Branches     930      930              
=========================================
+ Hits        2249     2250       +1     
+ Misses      2267     2266       -1
Impacted Files Coverage Δ
src/components/SecondaryPanes/Frames/index.js 69.86% <100%> (ø) ⬆️
src/actions/ast.js 78.57% <0%> (+1.78%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6fd4736...d447c15. Read the comment docs.

Copy link
Contributor

@codehag codehag left a comment

Choose a reason for hiding this comment

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

Looks good! wrote a comment to make sure I understand the fix. For the test, i would propose something like this:

describe("truncateFrames") {
  it("will truncate frames with FrameworkGrouping off") {
    const frameworkGroupingOn = false;
    const framesWithFramework = [ ... ] // datastructure
    const framesWithoutFramework = [ ... ] // datastructure
    const frames = [ ...framesWithFramework, ...framesWithoutFramework ];
    const { component } = render({ frameworkGroupingOn, frames });
    const button = component.find('button') // or whatever selector for that button

    button.simulate('click');
    expect(component.find('Frame').length).toBe(frames.length); // uncollapse
    button.simulate('click')
    expect(component.find('Frame').length).toBe(framesWithoutFramework.length + 1); // collapse
})

its a bit pseudocody... haven't tried it yet

I also tested this locally with this branch and the next branch. In the original issue it says that frames are missing, which ones should I look for?

@@ -154,7 +154,7 @@ class Frames extends Component {
? L10N.getStr("callStack.collapse")
: L10N.getStr("callStack.expand");

frames = collapseFrames(frames);
frames = this.collapseFrames(frames);
Copy link
Contributor

Choose a reason for hiding this comment

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

Was the issue that if frameworkGroupingOn was not checked, collapse frame would be run with unexpected results?

@jasonLaster jasonLaster merged commit b7615e6 into firefox-devtools:next Jul 25, 2017
jasonLaster added a commit to jasonLaster/debugger.html that referenced this pull request Jul 26, 2017
jasonLaster added a commit to jasonLaster/debugger.html that referenced this pull request Jul 27, 2017
jbhoosreddy pushed a commit to jbhoosreddy/debugger.html that referenced this pull request Aug 20, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants