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

Improve testing #230

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

fdemmer
Copy link

@fdemmer fdemmer commented Nov 22, 2022

The latest release broke a documented feature.

Regardless of how this situation will be solved, I propose to improve automated testing. Here are some of my suggested changes.

It includes a currently @unittest.skip() marked test (FormTests.test_form_condition_callable) which leads to a RecursionError due to using get_cleaned_data_for_step in a condition_dict callback.

@codecov
Copy link

codecov bot commented Nov 22, 2022

Codecov Report

Merging #230 (69cf190) into master (3eb4a43) will increase coverage by 0.38%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #230      +/-   ##
==========================================
+ Coverage   93.88%   94.26%   +0.38%     
==========================================
  Files          10       10              
  Lines         523      523              
  Branches       66       66              
==========================================
+ Hits          491      493       +2     
+ Misses         19       18       -1     
+ Partials       13       12       -1     
Impacted Files Coverage Δ
formtools/wizard/views.py 94.96% <0.00%> (+0.67%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@stuaxo
Copy link
Contributor

stuaxo commented Nov 22, 2022

This could probably be split up so it could be merged more easily.

I'm bound to have missed bits, but for instance there are improvements to tests on -

  • Cookies
  • File Uploads

If you make these separate PRs they can go in on their own + will be easier for others to evaluate; I haven't gone though the commits, but it may be you can create new PRs and cherry pick the relevant commits.

We may even want to split things up a bit in the test class, rather than having one large one.

Copy link
Contributor

@foarsitter foarsitter left a comment

Choose a reason for hiding this comment

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

Although @stuaxo is right that splitting this up would help I think we should merge this.

@claudep
Copy link
Contributor

claudep commented Apr 7, 2023

I guess anyone can make this go forward by creating distinct and reviewable PRs (keeping original author ownership, of course).

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