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.

Sign up for GitHub

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

Jump to bottom

Refactor testing to use the data structure directly #25

Merged
merged 3 commits into from
Jul 17, 2023
Merged

Refactor testing to use the data structure directly #25

merged 3 commits into from
Jul 17, 2023

Conversation

diazona
Copy link
Owner

@diazona diazona commented Jul 15, 2023

In this PR I'm splitting out most of the functionality of the plugin's run() method into a new method, _generate(), which returns the data structure corresponding to the contents of the pyproject.toml file but without actually writing it out as TOML. I've also modified the test infrastructure to give access to the return value of this method directly, through the new Project.generate() wrapper, so we can run tests without going through an unnecessary (ish?) round of writing the structure to TOML and then reading it back in.

I also copied all the existing tests but changed them to use this new generate() method. This introduces a bunch of duplicate tests. I think we can safely get rid of many of the original tests and just keep a few to make sure the whole process works, but most of our testing from now on can probably be done by using generate(). As I create this PR I'm not yet sure which/how many of the original tests it makes sense to remove, but we'll figure that out. @sjlongland I'd appreciate your thoughts on this.

@diazona diazona added this to the Initial release milestone Jul 15, 2023
@diazona diazona self-assigned this Jul 15, 2023
@diazona diazona force-pushed the refactor-generation/1/dev branch from f855175 to ba84406 Compare July 15, 2023 21:59
@diazona
Copy link
Owner Author

diazona commented Jul 15, 2023

I still have a bit of work to do to fix mypy's complaints about assignments to pyproject... I'm working on that. But other than the type annotations this should be ready.

@sjlongland
Copy link
Collaborator

Yeah, that would remove a source of potential test failures. It'd still be worth keeping a test that ensures that valid TOML is emitted regardless, but we'd then be able to make it just one single test and not multiple like we have now.

@diazona diazona mentioned this pull request Jul 16, 2023
Merged
diazona added 2 commits July 16, 2023 13:48
@diazona
Split run() into two parts for generating and writing the data
467b5fe 
This commit splits WritePyproject.run() into two methods, one that
generates the data structure corresponding to the content of
pyproject.toml and another that writes it out. This split will come in
handy for unit testing, so that we can write tests against _generate()
which don't have to go through the loop of writing out TOML-encoded
content and reading it back in.
@diazona
Refactor tests to use generate() directly
474125a 
In this commit I'm adding the Project.generate() method, which directly
returns the data structure generated by the plugin, without rendering it
to TOML. This gives us a more efficient way to test the plugin's output
without invoking or mimicing a subprocess and without going through
an unnecessary round of conversion to and back from TOML markup.

I duplicated all the existing tests which involve running setup.py with
script_runner.run(), except I changed the copies to use the generate()
method. We don't really need to have all these tests duplicated, but I'm
leaving it for a future commit to decide which ones from the original
batch to remove and which to keep. We'll definitely want to keep some of
the original tests that actually run setup.py, just to make sure that
the whole system does work, but going forward we can probably use
generate() to do the majority of our testing.

As part of this change, I added a call to monkeypatch.chdir() in
the project fixture so that every test which uses a project constructed
by that fixture will run in that project's root directory by default.
That's necessary because setuptools.run_setup() doesn't provide a way to
set the working directory itself like script_runner.run() does, so we
have to manually change to the right directory beforehand. I could have
added a call to chdir() as part of generate(), but changing directory as
part of a fixture allows pytest to restore the original directory after
the tests run, in case that's relevant.

In the future, perhaps we can also programmatically construct
Distribution objects instead of having to execute a setup.py file, but
that's an entirely separate feature.
@diazona diazona force-pushed the refactor-generation/1/dev branch from ba84406 to 474125a Compare July 16, 2023 20:48
@diazona diazona changed the base branch from main to precise-types/1/dev July 16, 2023 20:49
@diazona
Copy link
Owner Author

diazona commented Jul 16, 2023

Yay, I finally managed to satisfy mypy 😁 It was a bit tricky in this case.

@sjlongland Any thoughts about which of the original tests in test_setup_command.py we should keep to ensure that valid TOML is emitted? Personally, I'm thinking that it makes sense to keep one very basic test that emits a minimal pyproject.toml (like, just the project name or something), and one test that is as comprehensive as possible, emitting all the fields we have defined - of course we should keep updating that test as we add support for new fields. And honestly I'm not even sure of what sort of problem the basic test would detect; it's just my instinct that it might be useful to have as an edge case. 🤷 Let me know if you think there are other cases that would be useful to keep, and I'll push a commit that removes the ones we don't need. Or you could push that commit yourself, if you prefer.

Base automatically changed from precise-types/1/dev to main July 16, 2023 21:41
@sjlongland
Copy link
Collaborator

Yay, I finally managed to satisfy mypy grin It was a bit tricky in this case.

@sjlongland Any thoughts about which of the original tests in test_setup_command.py we should keep to ensure that valid TOML is emitted?
Personally, I'm thinking that it makes sense to keep one very basic test that emits a minimal pyproject.toml (like, just the project name or something), and one test that is as comprehensive as possible, emitting all the fields we have defined - of course we should keep updating that test as we add support for new fields.

test_name_and_version seems the simplest… the others are testing the logic in behind the generation of the data that is transformed to TOML, so would be good candidates to be re-worked to call the underlying generate() function.

I don't think there's one that emits everything yet.

And honestly I'm not even sure of what sort of problem the basic test would detect; it's just my instinct that it might be useful to have as an edge case. shrug Let me know if you think there are other cases that would be useful to keep, and I'll push a commit that removes the ones we don't need. Or you could push that commit yourself, if you prefer.

True, and if we keep the serialisation routine as dumb as possible, we're realistically not testing our own code, we're testing tomlkit, which should have its own tests.

Another approach is the call to tomlkit is its own method, which in unit tests, we stub and is itself excluded from test coverage (as we keep it "trivial"). That way, we just test what we feed into tomlkit, and we assert whatever we return from the stub, is processed correctly by the calling function.

It comes down to how much you trust the upstream project to be consistent in their output. :-)

@diazona
Copy link
Owner Author

diazona commented Jul 16, 2023

test_name_and_version seems the simplest… the others are testing the logic in behind the generation of the data that is transformed to TOML, so would be good candidates to be re-worked to call the underlying generate() function.

Makes sense. I'll push a commit that removes the others.

I don't think there's one that emits everything yet.

Yeah, I mean we haven't implemented support for most of the fields but I think there should be one test that emits all the fields we do have support for, and as we add support for new fields we just keep expanding the scope of that test. Sorry if I wasn't clear about that.

True, and if we keep the serialisation routine as dumb as possible, we're realistically not testing our own code, we're testing tomlkit, which should have its own tests.

My thoughts as well 👍

Another approach is the call to tomlkit is its own method, which in unit tests, we stub and is itself excluded from test coverage (as we keep it "trivial"). That way, we just test what we feed into tomlkit, and we assert whatever we return from the stub, is processed correctly by the calling function.

It comes down to how much you trust the upstream project to be consistent in their output. :-)

Hmm, interesting idea. I hadn't thought of that. I do think it makes the test code a little more complicated to follow, though probably not enough to be a big deal. FWIW I do trust tomlkit's output to be stable over the long term; or if it's not, we can probably adjust for that (if nothing else by parsing the output as you implemented before), but bypassing tomlkit entirely does save us that particular bit of effort.

@diazona
Drop down to two tests that actually run the setup.py command
b8bd0f6 
This commit changes the set of tests which actually run
`setup.py pyproject` to be just two: one extremely minimal test which
just has a name and version, and another comprehensive test that is
supposed to include all relevant fields that can appear in setup.cfg.
The idea is that as we add support for more fields, we'll adjust
the expected output of that test to keep up. I populated the dummy
setup.cfg file for the latter test with a bunch of fields that exist
in setuptools' own setup.cfg file as well as this project's setup.cfg.
@diazona diazona marked this pull request as ready for review July 17, 2023 00:59
@diazona diazona requested a review from sjlongland July 17, 2023 01:00
@diazona diazona assigned sjlongland and unassigned diazona Jul 17, 2023
@sjlongland sjlongland merged commit d77dc47 into main Jul 17, 2023
@sjlongland sjlongland deleted the refactor-generation/1/dev branch July 17, 2023 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sjlongland sjlongland sjlongland approved these changes

Assignees

@sjlongland sjlongland

Labels
None yet
Projects
None yet
Milestone
Initial release
Development

Successfully merging this pull request may close these issues.
2 participants
@diazona @sjlongland