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

Redux and Component tests, coverage. Normalize redux actions. #420

Merged
merged 21 commits into from
Jun 23, 2016

Conversation

TristanWright
Copy link
Contributor

@TristanWright TristanWright commented Jun 2, 2016

  • redux tests
    • for: Clusters, Projects & Simulations, and Taskflows:
    • simple actions
    • simple reducers
  • component tests?

@TristanWright TristanWright force-pushed the tests-1 branch 5 times, most recently from f34a55b to e6b95c6 Compare June 6, 2016 18:51
@TristanWright
Copy link
Contributor Author

Cannot figure out why these two tests are failing:
https://travis-ci.org/Kitware/HPCCloud/jobs/135997229#L2007

@TristanWright
Copy link
Contributor Author

Because of the odd async behavior, I'm thinking of just testing simple actions (actions which return { type: ..., someData: ... }) and the corresponding reducer. If so, redux bugs here out may be because we're calling a simple action in an async action where we don't intend to, (the effect of the action will be correct though we'll just be mistakingly calling it.)

@dmitry-zaets
Copy link

dmitry-zaets commented Jun 8, 2016

Since you are using expect - it contains spy functions.
Basic idea is that you can mock any function by creating spy on it.
This will allow you to mock client.listSimulations(id) and return fake promise with data or with error, for different test cases.The easiest way to do mocks is in beforeEach part of Jasmine tests.

Here is an example:

beforeEach(function () {
  const response = { data: 'your data'};
  expect.spyOn(client, 'listSimulations')
    .andReturn(Promise.resolve(response));
})

afterEach(function () {
  expect.restoreSpies(); // This is needed for restoring default function behaviour
})

it('works', function (done) {
  expect(fetchProjectSimulations())
    .toDispatchActions([ {type: 'test1'}, {type: 'test2'} ]);
})

@TristanWright
Copy link
Contributor Author

@dmitry-zaets THANKS! Hadn't thought of that and I don't have to intercept xhr requests anymore. So happy with this setup now.

@TristanWright TristanWright force-pushed the tests-1 branch 3 times, most recently from c27946b to 1fe0047 Compare June 14, 2016 16:47
@TristanWright
Copy link
Contributor Author

Cool we have codecov : https://codecov.io/gh/Kitware/HPCCloud/branch/tests-1

@cjh1
Copy link

cjh1 commented Jun 15, 2016

Awesome! Well done.

@cjh1
Copy link

cjh1 commented Jun 20, 2016

@TristanWright Let rename the PR to something a little more descriptive :-) The changes to the redux code, are these the results of problems uncovered by the testing? Its not clear exactly what they fix, so makes it a little hard to review.

@TristanWright TristanWright changed the title Tests 1 Redux and Component tests and coverage Jun 20, 2016
@TristanWright TristanWright changed the title Redux and Component tests and coverage Redux and Component tests, coverage. Normalize redux actions. Jun 20, 2016
@TristanWright
Copy link
Contributor Author

Renamed PR,
Redux changes normalize our redux actions, for some actions what we'd return was inconsistent. Like actions that have some async component need to dispatch and sometimes didn't. (https://github.com/Kitware/HPCCloud/pull/420/files#diff-3cdd813e5fad39baac221af549782a5bL30)

@TristanWright
Copy link
Contributor Author

TristanWright commented Jun 20, 2016

@cjh1
Copy link

cjh1 commented Jun 21, 2016

Thanks for the explanation, I will try to test this out today.

@cjh1
Copy link

cjh1 commented Jun 21, 2016

When running the test ( which seem to pass ) I am seeing the following:


ERROR LOG: 'Unhandled promise rejection', TypeError{stack: 'http://localhost:9876/base/test/contexts/tests.all.js?392a3be52600af9141667a9059cfaa12e61b93f4:23375:10466
run@http://localhost:9876/base/node_modules/babel-polyfill/dist/polyfill.js?445a020deba63c9118cf9c570f97130c461f14d7:3931:29
http://localhost:9876/base/node_modules/babel-polyfill/dist/polyfill.js?445a020deba63c9118cf9c570f97130c461f14d7:3944:31
flush@http://localhost:9876/base/node_modules/babel-polyfill/dist/polyfill.js?445a020deba63c9118cf9c570f97130c461f14d7:1214:11', line: 23375, sourceURL: 'http://localhost:9876/base/test/contexts/tests.all.js?392a3be52600af9141667a9059cfaa12e61b93f4'}

Is this expected?


return action;
return action;
};
}
Copy link

Choose a reason for hiding this comment

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

I assume these are just formatting fixes? In future it might be go to do the formatting changes in a separate commit, to make it easier to see the logic changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The block is wrapped in a (dispatch) => {} now. I'll be sure to keep formatting separate.

@TristanWright
Copy link
Contributor Author

When running the test ( which seem to pass ) I am seeing the following:

ERROR LOG: 'Unhandled promise rejection', TypeError{stack: 'http://localhost:9876/base/test/contexts/tests.all.js?392a3be52600af9141667a9059cfaa12e61b93f4:23375:10466
run@http://localhost:9876/base/node_modules/babel-polyfill/dist/polyfill.js?445a020deba63c9118cf9c570f97130c461f14d7:3931:29
http://localhost:9876/base/node_modules/babel-polyfill/dist/polyfill.js?445a020deba63c9118cf9c570f97130c461f14d7:3944:31
flush@http://localhost:9876/base/node_modules/babel-polyfill/dist/polyfill.js?445a020deba63c9118cf9c570f97130c461f14d7:1214:11', line: 23375, sourceURL: 'http://localhost:9876/base/test/contexts/tests.all.js?392a3be52600af9141667a9059cfaa12e61b93f4'}

Is this expected?

I've tried to track it down several times, by looking through the stack trace there and commenting in and out test blocks to see if it's coming from one specifically. I think it's related to some combination of how we're polyfilling Promises in PhantonJS. If we run the tests in Chrome we don't get any of the same errors. There's a headless chromium in development, when it's ready we'll switch to it. issue 546953

@cjh1
Copy link

cjh1 commented Jun 21, 2016

I am trying to create a trad cluster and it never seems to move into the running state. I just see the following in the console:

cluster creating

This could be something up with environment, can you confirm you are able to create a cluster?

@TristanWright
Copy link
Contributor Author

When I re-up my vm I usually I need to restart girder. I appended some cluster preference changes last night too if you haven't pulled.

@cjh1
Copy link

cjh1 commented Jun 21, 2016

This was indeed a local configuration issue.

@TristanWright
Copy link
Contributor Author

There are going to be some conflicts with pull #409, this should go first though.

@cjh1
Copy link

cjh1 commented Jun 22, 2016

@TristanWright Your last commit does fix the new cluster creation issue. However, I still see an issue with the state transition from "created" to "running" a refresh is required.

@TristanWright
Copy link
Contributor Author

This confusion came from about how the cluster store items list and mapById are related and used.

Added redux store documentation to issue #406 (Documentation).

@cjh1
Copy link

cjh1 commented Jun 22, 2016

I am also seeing that in the "Simulation" view a newly create EC2 cluster doesn't appear without a page refresh, not sure if this is new behavior.

@TristanWright
Copy link
Contributor Author

Simulation start or run? I can run and the log and jobs appear

On Wed, Jun 22, 2016 at 3:03 PM, Chris Harris notifications@github.com
wrote:

I am also seeing that in the "Simulation" view a newly create EC2 cluster
doesn't appear without a page refresh, not sure if this is new behavior.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#420 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AMs36V8u4EIcNZd-UiWuSxDFFN6soPCsks5qOaMmgaJpZM4Is4XJ
.

  • Tristan Wright

R&D Engineer
Kitware Inc.

@cjh1
Copy link

cjh1 commented Jun 22, 2016

This is when starting a simulation ( creating a new ec2 cluster )

@TristanWright
Copy link
Contributor Author

I'm not sure I'm seeing it. I do recall some issue like this, for now I
wouldn't say it's new.

On Wed, Jun 22, 2016 at 3:15 PM, Chris Harris notifications@github.com
wrote:

This is when starting a simulation ( creating a new ec2 cluster )


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#420 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AMs36RaiOQQ4pjUfNPup0E5L8jjdd1Zsks5qOaXmgaJpZM4Is4XJ
.

  • Tristan Wright

R&D Engineer
Kitware Inc.

@cjh1
Copy link

cjh1 commented Jun 23, 2016

I tested against master, it not new.

LGTM

@TristanWright
Copy link
Contributor Author

excellent!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants