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

Disable ruleset tests #128

Merged
merged 2 commits into from
Jun 11, 2018
Merged

Disable ruleset tests #128

merged 2 commits into from
Jun 11, 2018

Conversation

andylolz
Copy link
Contributor

@andylolz andylolz commented Jun 7, 2018

Profiling suggests this test is one of the slowest. It doesn’t appear to be used by the dashboard anyway. So disabling it should speed things up a bit, without causing issues for the dashboard.

Refs #129.

@coveralls
Copy link

coveralls commented Jun 7, 2018

Coverage Status

Coverage remained the same at 55.367% when pulling 7f1cb9f on andylolz:rm-ruleset-tests into 5b3d85a on IATI:master.

@andylolz
Copy link
Contributor Author

andylolz commented Jun 7, 2018

Some crude benchmarking:

$ git checkout master
$ time python calculate_stats.py loop --folder millenniumchallenge

real    1m30.076s
user    1m26.325s
sys     0m2.056s

Re-running on this branch:

$ git checkout rm-ruleset-tests
$ time python calculate_stats.py loop --folder millenniumchallenge

real    1m5.361s
user    1m3.713s
sys     0m1.257s

So that’s about 17% faster.

dalepotter added a commit to IATI/IATI-Dashboard that referenced this pull request Jun 11, 2018
Underlaying json stats file was removed in IATI/IATI-Stats#128
@dalepotter
Copy link
Contributor

dalepotter commented Jun 11, 2018

Looking at the Dashboard code, it seems that there was an intention to use this on in this page, which for some reason was never decided to be made part of the live site. The non-public page can be previewed locally here (with old data):

image

Given the time needed to generate underlaying data as shown above, I'm approving this PR and have set up a new PR to remove the code from the Dashboard: IATI/IATI-Dashboard#490

@bill-anderson - as you were around at the time of the Dashboard build, let me know if rulesets not being included is a mistake somewhere, in which case this (and IATI/IATI-Dashboard#490) can be reverted.

Copy link
Contributor

@dalepotter dalepotter left a comment

Choose a reason for hiding this comment

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

👍

@dalepotter dalepotter merged commit af157bb into IATI:master Jun 11, 2018
@andylolz andylolz deleted the rm-ruleset-tests branch June 11, 2018 15:36
@andylolz
Copy link
Contributor Author

It’s also worth removing these ruleset tests because they’re currently very broken. See: IATI/IATI-Rulesets#76.

@andylolz
Copy link
Contributor Author

@bill-anderson - as you were around at the time of the Dashboard build, let me know if rulesets not being included is a mistake somewhere

As mentioned on IATI/IATI-Dashboard#490: See IATI/IATI-Dashboard#108 for more information on this. Rulesets were removed from the dashboard way back in IATI/IATI-Dashboard@fa81205.

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.

3 participants