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

InlinePanel - add unit test coverage #9155

Open
Tracked by #9145
lb- opened this issue Sep 6, 2022 · 4 comments
Open
Tracked by #9145

InlinePanel - add unit test coverage #9155

lb- opened this issue Sep 6, 2022 · 4 comments

Comments

@lb-
Copy link
Member

lb- commented Sep 6, 2022

Is your proposal related to a problem?

  • InlinePanel and the related buildExpandingFormset utils are quite fragile and can break in cases where the DOM structure changes or in more complex cases like nested panels easily.

Describe the solution you'd like

  • Unit test coverage should help us be a bit more confident about changes and understanding the behaviour.
  • It might be suitable to import the html template for testing this also.

Describe alternatives you've considered

  • Integration tests or browser testing could also be suitable for this or the pattern library.

Additional context

@lb- lb- changed the title Inline Panel - add unit test coverage InlinePanel - add unit test coverage Sep 6, 2022
@spapas
Copy link
Contributor

spapas commented Sep 17, 2022

Hey @lb- I was just going to open some discussion about that particular thingie! The issue #9145 is way too big and affected everybody that upgraded to wagtail 4.x! This shouldn't be taken lightly.

The fact that a bug this important slipped from testing means that there's something flawed in the way new wagtail releases are tested; adding some tests for inlinepanels is definitely a good first step but what about other similar issues? The whole wagtail-admin seems way too fragile too me because of the javascript-python trickery it includes so how can anybody be reassured that other things won't break in next updates?

It feels that people that update to new wagtail versions when they are released are actually doing beta-testing without their consent!

I really don't know how this could be improved; however I feel that some discussion with the Wagtail team would be good. Maybe avoid such big changes? Maybe don't allow changes that have a probability of breaking things? Maybe add even more integration tests? Maybe give more priority to testing (i.e do not add any more features to the project until everything has been covered by tests)?

Thank you and kind regards,
Serafeim

@zerolab
Copy link
Contributor

zerolab commented Sep 17, 2022

@spapas thank you very much for the input.

I'd like to note that if we steer away from making big changes, then we will end up making no changes, or very little progress.

We do lots of manual testing when reviewing PRs, but we're also only human.. I don't seen an issue with the Python + JavaScript "trickery", but we do want to increase our coverage and this is one of those steps.

FWIW, the version jump came from the fact we had so many changes and it is a way to indicate further testing may be needed before just upgrading. Heh, ideally we would just be able to do the version bump and everything else would just work, but we cannot and do not want to control what and how people build their sites, or what packages they use or don't use. There is a reason we do a couple of weeks of release candidate (and sometimes 2 release candidates) and ask people to test with their projects.

Regardless, this is not something that we just take lightly. And it most certainly something that we can and should improve. We cannot do it alone, and we do need help from the community. From providing constructive feedback, to bug fixes, to doing the real-life testing (sometimes even with the nightly builds, or release candidates), to keeping packages up to date, and.. helping us achieve the "everything has been covered by tests" state.

Finally, we can probably continue this in a discussion, as it would be great to hear the community experience with the big releases, the upgrade process, as well as constructive suggestions based on experience or practice with other software

@spapas
Copy link
Contributor

spapas commented Sep 17, 2022

Hello @zerolab thank you for understanding the situation and taking it seriously. Also forgive me for hijacking LB's thread ; I wasn't aware the discussions feature is used for Wagtail, I'd keep it in mind from now on...

I'd be grateful if the testing and release process is actually improved and similar bugs are catched in the future before reaching production sites. This will definitely make Wagtail updates less problematic and imrove Wagtail as a framework in general.

Kind regards,
Serafeim

PS I think that it's better to not release a new version than releasing something that introduces (or has a probability of introducing) bugs to production apps.

@lb-
Copy link
Member Author

lb- commented Sep 17, 2022

@spapas thanks for the input and I hear your frustration.

I agree 100% with everything @zerolab said - including that further discussion should be a standalone discussion about automated or manual ways to pick up things sooner.

There are plans underway (beyond just this issue) for some other testing layers to be added. All of these take time and resources and may not be prioritised and unlikely to be sponsored so there is no locked in roadmap for these.

  • Storybook testing support (good line between integration tests and unit tests).
  • More integration tests (we already have some for critical accessibility bugs).
  • RFC78 to simplify and abstract some of the easy gotchas when working with 'JavaScript trickery' (for example this exact bug would never be an issue with Stimulus' approach to targeting scoped elements in the DOM).

Thanks again Serafeim.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 📋 Backlog
Development

No branches or pull requests

3 participants