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

Integration test suite #731

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Integration test suite #731

wants to merge 4 commits into from

Conversation

tmbb
Copy link

@tmbb tmbb commented Dec 27, 2024

This is not ready to be merged, and it contains some irrelevant changes (for this functionality) which I can remove if its time for merging

Currently, Backpex has no integration tests (at least, not integration tests that you can run with mix test). Unfortunately, the only way of running integration tests with Backpex, which requires creating a Phoenix app inside the test suite. I have created a phoenix app inside the test suite as much as possible (some parts are still outside the tests suite, such as assets, as I haven't figured out how to move the assets' location). I have added new config files, and I don't think it's easy to compile a phoenix app without those.

If Backpex is compiled in :dev or :test it will now create a phoenix app which can be visited (to visually check everything is ok) and inspected in automated tests. Instead of testing with a real browser engine, which is quite heavy, I'm using the excelent PhoenixTest library, which doesn't run any javascript but which can test the behaviour of vieviews by spawning the appropriate evengs and mimicking mouse clicks and such.

I plan on increasing test coverage progressively (this first commit only tests the emtpy index pages for the resources), but setting up the phoenix app is actually the hardest part.

Main changes

Define a phoenix app in the test suite

The test suite now includes a phoenix app. The app is configured using config/*.exs files at the top level (I'm not sure it can be configured any other way) and uses assets defined in the top-level assets/ directory (again, I'm not sure it can be done any other way.

Turn the form labels into real labels

Currently, form labels are defined using <p> elements. This is not semantically correct, and without a for={...} attribute, one can't know programmatically which element the label points too. This is probably bad from an accessibility perspective (probably, I'm not totally aware of modern accessibility guidelines), and it's certainly bad from the perspective of integration testing. I've started using a very cool library called PhoenixTest, which unfortunately only allows filling in input values with labels (the idea is that since the human user uses the label as a guide, the testing framework should use the same).

For these two reasons, I have updated the input_label(assigns) component to take a field as input instead of only the label text. It now renders a <label> instead of <p> elements.

Testing Ash-related functionality

I have not implemented any Ash-related tests because I know nothing about Ash.

Should this be merged eventually?

From what I can gather while reading Backpex's source, I'm not sure these changes are within the spirit of what the Backpex developpers want, and I'm happy to continue using my fork (or even rename the library and publish it as something else) if the developers prefer not to deal with these (admittedly very disruptive) PRs.

So if the Backpex developpers don't think these PRs are appropriate, please say so and I'll just fork Backpex into something else and work within my fork only.

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.

1 participant