Skip to content

Conversation

brunobord
Copy link
Contributor

@brunobord brunobord commented Feb 13, 2018

As you may see, this PR is establishing a set of basic tests on the Form schema, based on the formidable.yml file that lives in the documentation.
From this perspective, I'm only testing it as-is, and I've added a certain number of "FIXME" tags to enhance this definition and fix it to synchronize it with the truth (ie.: the runtime code)

Review

  • Tests
  • docs/comments
  • CHANGELOG.rst Updated
  • Delete your branch

@brunobord brunobord force-pushed the formidable-schema-test-on-yml branch from 18b17cd to aa2725f Compare February 13, 2018 15:04
@brunobord brunobord force-pushed the formidable-schema-test-on-yml branch 2 times, most recently from 02813f0 to 97a3521 Compare February 13, 2018 15:56
Copy link
Contributor

@alexdashkov alexdashkov left a comment

Choose a reason for hiding this comment

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

it seems to be good.
Also, I noticed that you have to use pytest in the requirements, how about to switch to it completely?


def test_wrong_type_fields_int():
form = _load_fixture('0010_wrong_type_field_int.json')
errors = sorted(validator.iter_errors(form), key=lambda e: e.path)
Copy link

Choose a reason for hiding this comment

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

This line occurs very often, have you considered a helper function to improve readability?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

now that you're mentioning it...
I'll see how I can improve this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mmmm... thinking about it, I'd be replacing one line of code by... one line of code & a function definition + one line of code... is it worth it?

Copy link

Choose a reason for hiding this comment

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

I was thinking this line is a bit "heavy" to read, and replacing it by a function call could make things clearer. But it definitely requires more code... Your call, the PR is approved 👍

@@ -0,0 +1,6 @@
{
Copy link

Choose a reason for hiding this comment

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

any reason the number in the file name goes from 0004 to 0010? (also some numbers are similar)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried to work using "blocks" of similar tests and "space" them to make sure I could squeeze more tests in the blocks without having to rename all my fixtures.
The numbers are following the schema definition logic, from the most basic to the most complicated.
On top of that, they haven't been written in order, I've jumped from one idea to another, so I've put "spaces" when I was starting to write a test that was a bit ahead, so sometimes there's a gap between fixture numbers.

Copy link

Choose a reason for hiding this comment

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

Thanks for the answer :)

@brunobord
Copy link
Contributor Author

Also, I noticed that you have to use pytest in the requirements, how about to switch to it completely?

this one was a bit different. I was just needing pure python tests, and nothing related to Django, so I didn't have to install pytest+pytest-django and try to configure this.

But we may try to open this topic during the next technical grooming.

This YAML file defines the accepted methods and formats for the form builder. It appeared that this is incomplete or obsolete and requires to be tested. As soon as something changes in this form definition, we should reflect it using a test that shows the impact.
It is recommended to use the TDD method, by writing the test first and implement the change on your second move.

The present test files are reflecting the spec *as it is written now* and will probably evolve as we'll fix them regarding the Python code that is supposed to implement this spec.
valid python 2.7 code is probably safe flake8-wise. It avoids hitting "syntax errors" when checking pure python3.6 test code.
@brunobord brunobord force-pushed the formidable-schema-test-on-yml branch from 97a3521 to e8f6295 Compare February 14, 2018 10:15
@brunobord brunobord merged commit e8f6295 into master Feb 14, 2018
@brunobord brunobord deleted the formidable-schema-test-on-yml branch February 14, 2018 11:12
brunobord added a commit that referenced this pull request Feb 21, 2018
**Deprecation Warning**: The validation endpoint (using the URL ``forms/(?P<pk>\d+)/validate/``) is now ``POST`` only.

- Added tests against the ``formidable.yml`` schema definition of Forms (#295).
- Fixed various items in the schema definition (#297).
- Validation endpoint for **user data** doesn't allow GET method anymore (#300).
- Add support for multiple conditions to target a common field.
@brunobord brunobord mentioned this pull request Feb 21, 2018
7 tasks
brunobord added a commit that referenced this pull request Feb 21, 2018
**Deprecation Warning**: The validation endpoint (using the URL ``forms/(?P<pk>\d+)/validate/``) is now ``POST`` only.

- Added tests against the ``formidable.yml`` schema definition of Forms (#295).
- Fixed various items in the schema definition (#297).
- Validation endpoint for **user data** doesn't allow GET method anymore (#300).
- Add support for multiple conditions to target a common field.
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