search

openfisca / legislation-explorer

Explore legislation formulas and parameters.
https://legislation.demo.openfisca.org
GNU Affero General Public License v3.0
26 stars 12 forks source link

Use new param api #70

Closed fpagnoux closed 7 years ago

fpagnoux commented 7 years ago

Connected to #69

fpagnoux commented 7 years ago

Ideally, I would have liked to extend the integration tests, as they currently do not cover parameters (only variables). I tried on branch extend-integration-tests, but the test doesn't pass, and I get a DetailsComponent.description was not found. while the page seem to be properly rendered in the browser. I don't have time to investigate more, so I'm opening the PR now.

fpagnoux commented 7 years ago

I'm working on integration tests.

fpagnoux commented 7 years ago

Ok, done with the tests and it's working (I must have had a bug with selenium server).

@cbenz there is a problem with the CIRCLE tests though: only one container pass, the 3 others fail with The Selenium server could not be reached. Will this be solve by #71 ?

cbenz commented 7 years ago

@fpagnoux Yes, that's why I recommend merging #71 first.

fpagnoux commented 7 years ago

@cbenz we didn't test with the same API ! I was using the one on prod.

I improved the robustness so that we don't have any error when https://github.com/openfisca/openfisca-web-api/pull/116 is merged, but I chose to not render scale parameters in this PR.

I think having a temporary solution to display the simple parameters via the flask API and the scales via the old one is too much work, as I assume we probably will adapt the scales rendering to the flask API in the next iteration.

cbenz commented 7 years ago

@fpagnoux You may now rebase on master to see integration tests happen in SauceLabs!

fpagnoux commented 7 years ago

Considering that:

I'm dismissing the review and moving forward.

PS: If it breaks the prod, I intend to blame the tests 😈