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

Don't mutate Pane serialized state during deserialization #18222

Merged
merged 1 commit into from Oct 15, 2018

Conversation

Projects
None yet
3 participants
@tjfryan

tjfryan commented Oct 11, 2018

Identify the Bug

#18221

Description of the Change

Don't Object.assign into the serialized state object.

Alternate Designs

none

Possible Drawbacks

None I can imagine unless this strange behavior was relied upon for some reason.

Verification Process

atom --test spec resulted in the following output

Project
  .replace
    it clears a project through replace with no params
      Expected 'buzz' to be undefined.
        at jasmine.Spec.it (/Users/tjfryan/atom/spec/project-spec.js:304:38)
      Expected [ '/var/folders/8h/s7w820y96yj0k72y8dbjlzgx234zj0/T/project-path1118910-8465-1x2mi1x.8u3l', '/var/folders/8h/s7w820y96yj0k72y8dbjlzgx234zj0/T/project-path2118910-8465-1l4r37f.hzpl' ] to equal [  ].
        at jasmine.Spec.it (/Users/tjfryan/atom/spec/project-spec.js:305:39)
    it responds to change of project specification
      Expected false to be true.
        at jasmine.Spec.it (/Users/tjfryan/atom/spec/project-spec.js:318:25)


Finished in 447.432 seconds
2088 tests, 10986 assertions, 3 failures, 2 skipped

None of the failures appear to be related

John Ryan
@lee-dohm

This comment has been minimized.

Member

lee-dohm commented Oct 15, 2018

@daviwil Do you want to rerun this and merge if possible?

@daviwil

This comment has been minimized.

Member

daviwil commented Oct 15, 2018

@daviwil

This comment has been minimized.

Member

daviwil commented Oct 15, 2018

Strangely that build failed due to a different state-related test:

2018-10-15T18:00:23.5036079Z   1) AtomApplication launch persists window state based on the project directories:
2018-10-15T18:00:23.5036141Z      AssertionError: expected [ 'Hello World!' ] to include 'Hello World! How are you?'
2018-10-15T18:00:23.5036209Z       at Function.assert.include (D:\a\1\s\node_modules\chai\lib\chai\interface\assert.js:843:45)
2018-10-15T18:00:23.5036254Z       at Context.it (D:\a\1\s\spec\main-process\atom-application.test.js:236:14)
2018-10-15T18:00:23.5036293Z       at <anonymous>:null:null

I've restarted it one more time, if there's another failure like this we may need to investigate:

https://github.visualstudio.com/Atom/_build/results?buildId=15832&view=logs

@daviwil

This comment has been minimized.

Member

daviwil commented Oct 15, 2018

Ran it a couple more times and it's green. Merging this, thanks @tjfryan!

@daviwil daviwil merged commit d1fd0d2 into atom:master Oct 15, 2018

3 checks passed

Atom Pull Requests #15863 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment