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

ability to sort and filter on actions #497

Merged
merged 3 commits into from Oct 10, 2018

Conversation

Projects
None yet
3 participants
@peterbe
Contributor

peterbe commented Oct 8, 2018

Fixes #151

@peterbe peterbe requested a review from rehandalal Oct 8, 2018

@peterbe

This comment has been minimized.

Show comment
Hide comment
@peterbe

peterbe Oct 8, 2018

Contributor

If you're wondering, I rebased this after I realized that my fancy new yarn 1.10 had added integrity values to all packages in the yarn.lock.

Contributor

peterbe commented Oct 8, 2018

If you're wondering, I rebased this after I realized that my fancy new yarn 1.10 had added integrity values to all packages in the yarn.lock.

@connect(
(state, props) => ({
actions: getAllActions(state, new Map()),

This comment has been minimized.

@peterbe

peterbe Oct 8, 2018

Contributor

Can/should this be actions: getAllActions(state) || new Map() instead?

@peterbe

peterbe Oct 8, 2018

Contributor

Can/should this be actions: getAllActions(state) || new Map() instead?

This comment has been minimized.

@rehandalal

rehandalal Oct 10, 2018

Member

it could be that too.

@rehandalal

rehandalal Oct 10, 2018

Member

it could be that too.

@peterbe

This comment has been minimized.

Show comment
Hide comment
@peterbe

peterbe Oct 8, 2018

Contributor

screen shot 2018-10-08 at 12 38 35 pm

There might be a way to make that drop-down be a multi-value but I don't know how and I'm just not sure if it's worth it. If I tried to make it an array instead the XHR requests would become /api/v2/recipe/?ordering=-last_updated&action=console-log,preference-experiment&page=1 (not the comma-separated list of action names)

Contributor

peterbe commented Oct 8, 2018

screen shot 2018-10-08 at 12 38 35 pm

There might be a way to make that drop-down be a multi-value but I don't know how and I'm just not sure if it's worth it. If I tried to make it an array instead the XHR requests would become /api/v2/recipe/?ordering=-last_updated&action=console-log,preference-experiment&page=1 (not the comma-separated list of action names)

const filters = this.getFilters();
const filterIds = Object.keys(filters).map(key => `${key}-${filters[key]}`);
const requestId = `fetch-filtered-recipes-page-${pageNumber}-${filterIds.join('-')}`;

This comment has been minimized.

@peterbe

peterbe Oct 8, 2018

Contributor

The change here is that I moved it from being a static object of functions to instead being defined inside the render method. I did that so I can dynamically (using closures) get the list of all possible action names. And I also, changed the function for action. The rest is just code moves.

@peterbe

peterbe Oct 8, 2018

Contributor

The change here is that I moved it from being a static object of functions to instead being defined inside the render method. I did that so I can dynamically (using closures) get the list of all possible action names. And I also, changed the function for action. The rest is just code moves.

@peterbe

This comment has been minimized.

Show comment
Hide comment
@peterbe

peterbe Oct 8, 2018

Contributor

@rehandalal @mythmon Can you help me out here. Look at https://circleci.com/gh/mozilla/delivery-console/3531

The last line of the output says: Too long with no output (exceeded 10m0s) and I can't open the logs in raw. I'm going to try to ssh in to run it locally to see what's going on.

Also, why are some unit tests taking 45 seconds to run? Are we doing too much concurrent test runs that it's CPU thrashing?

screen shot 2018-10-08 at 1 37 46 pm

Contributor

peterbe commented Oct 8, 2018

@rehandalal @mythmon Can you help me out here. Look at https://circleci.com/gh/mozilla/delivery-console/3531

The last line of the output says: Too long with no output (exceeded 10m0s) and I can't open the logs in raw. I'm going to try to ssh in to run it locally to see what's going on.

Also, why are some unit tests taking 45 seconds to run? Are we doing too much concurrent test runs that it's CPU thrashing?

screen shot 2018-10-08 at 1 37 46 pm

@peterbe

This comment has been minimized.

Show comment
Hide comment
@peterbe

peterbe Oct 8, 2018

Contributor

Ah! One clue is that we're running out of memory:
screen shot 2018-10-08 at 1 44 04 pm

Oh jest!

I'm going to see if we can run the tests in serial or something like that.

Contributor

peterbe commented Oct 8, 2018

Ah! One clue is that we're running out of memory:
screen shot 2018-10-08 at 1 44 04 pm

Oh jest!

I'm going to see if we can run the tests in serial or something like that.

@peterbe

This comment has been minimized.

Show comment
Hide comment
@peterbe

peterbe Oct 8, 2018

Contributor

The number of CPUs becomes uncontrollably big which isn't right when you use circleci/python:3.6-node-browsers

See:

> circleci@9e4c489cf76b:~/repo$ node
> var os = require('os')
undefined
> os.cpus().length
36

When I, arbitrarily, pick max 2 workers things work better:

> circleci@9e4c489cf76b:~/repo$ time yarn run test:ci --maxWorkers 2
yarn run v1.9.4
$ CI=true yarn test --maxWorkers 2
$ node scripts/test.js test --env=jsdom --maxWorkers 2
...
 PASS  src/tests/components/data/QueryExtension.test.js
 PASS  src/tests/components/data/QueryExtensionListingColumns.test.js
 PASS  src/tests/components/data/QueryRecipeListingColumns.test.js
 PASS  src/tests/components/forms/FormActions.test.js
 PASS  src/tests/utils/router.test.js
 PASS  src/tests/utils/api.test.js

Test Suites: 68 passed, 68 total
Tests:       236 passed, 236 total
Snapshots:   0 total
Time:        11.939s
Ran all test suites matching /test/i.
Done in 13.07s.

real	0m13.345s
user	0m24.808s
sys	0m2.792s
Contributor

peterbe commented Oct 8, 2018

The number of CPUs becomes uncontrollably big which isn't right when you use circleci/python:3.6-node-browsers

See:

> circleci@9e4c489cf76b:~/repo$ node
> var os = require('os')
undefined
> os.cpus().length
36

When I, arbitrarily, pick max 2 workers things work better:

> circleci@9e4c489cf76b:~/repo$ time yarn run test:ci --maxWorkers 2
yarn run v1.9.4
$ CI=true yarn test --maxWorkers 2
$ node scripts/test.js test --env=jsdom --maxWorkers 2
...
 PASS  src/tests/components/data/QueryExtension.test.js
 PASS  src/tests/components/data/QueryExtensionListingColumns.test.js
 PASS  src/tests/components/data/QueryRecipeListingColumns.test.js
 PASS  src/tests/components/forms/FormActions.test.js
 PASS  src/tests/utils/router.test.js
 PASS  src/tests/utils/api.test.js

Test Suites: 68 passed, 68 total
Tests:       236 passed, 236 total
Snapshots:   0 total
Time:        11.939s
Ran all test suites matching /test/i.
Done in 13.07s.

real	0m13.345s
user	0m24.808s
sys	0m2.792s
@peterbe

This comment has been minimized.

Show comment
Hide comment
@peterbe

peterbe Oct 8, 2018

Contributor

See the latest commit ^. I picked 2 because that's what someone suggest here: https://discuss.circleci.com/t/memory-problems-with-jest-and-workers/10297/4

Shame there's no way to document this in package.json.

Contributor

peterbe commented Oct 8, 2018

See the latest commit ^. I picked 2 because that's what someone suggest here: https://discuss.circleci.com/t/memory-problems-with-jest-and-workers/10297/4

Shame there's no way to document this in package.json.

@peterbe

This comment has been minimized.

Show comment
Hide comment
@peterbe

peterbe Oct 8, 2018

Contributor

Wonderful! Now the test block only takes 20 seconds.

Contributor

peterbe commented Oct 8, 2018

Wonderful! Now the test block only takes 20 seconds.

@mythmon

This comment has been minimized.

Show comment
Hide comment
@mythmon

mythmon Oct 8, 2018

Member

I imagine that CI is running on a docker-host that really does have 36 physical cores, but we are limited by a CPU quota. That's pretty normal for Docker. It's too bad that Jest didn't pick up on this automatically. Maybe we should file a bug for that? And even if we did have access to all 36 cores, we clearly don't have the memory to support that. It's probably less reasonable to ask Jest to autodetect that for us.

Nice work spotting that we were running with too many parallel jobs. I don't think I would have thought to check that.

Member

mythmon commented Oct 8, 2018

I imagine that CI is running on a docker-host that really does have 36 physical cores, but we are limited by a CPU quota. That's pretty normal for Docker. It's too bad that Jest didn't pick up on this automatically. Maybe we should file a bug for that? And even if we did have access to all 36 cores, we clearly don't have the memory to support that. It's probably less reasonable to ask Jest to autodetect that for us.

Nice work spotting that we were running with too many parallel jobs. I don't think I would have thought to check that.

@rehandalal

nice work on this!

and thanks for diving into the jest issue!!

bors r+

@connect(
(state, props) => ({
actions: getAllActions(state, new Map()),

This comment has been minimized.

@rehandalal

rehandalal Oct 10, 2018

Member

it could be that too.

@rehandalal

rehandalal Oct 10, 2018

Member

it could be that too.

bors bot added a commit that referenced this pull request Oct 10, 2018

Merge #497
497: ability to sort and filter on actions r=rehandalal a=peterbe

Fixes #151

Co-authored-by: Peter Bengtsson <mail@peterbe.com>
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors bot Oct 10, 2018

Contributor
Contributor

bors bot commented Oct 10, 2018

@bors bors bot merged commit 9c55f9f into mozilla:master Oct 10, 2018

3 checks passed

bors Build succeeded
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: docs Your tests passed on CircleCI!
Details

@peterbe peterbe deleted the peterbe:151-ability-to-sort-and-filter-on-actions branch Oct 10, 2018

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