Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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-levelassets/
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 afor={...}
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 calledPhoenixTest
, 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.