Skip to content

Convert specs to minitest/spec, get to 100% line/branch coverage #292

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

Merged
merged 23 commits into from
May 4, 2022

Conversation

jeremyevans
Copy link
Contributor

This makes the specs faster and more consistent with Rack's own specs.

Fix various issues uncovered by coverage testing.

Extend Ruby version support down to 2.0, since the existing specs run fine on Ruby 2.0.

Add CI testing on JRuby 9.3.

jeremyevans added 16 commits May 4, 2022 10:00
Advantages:

* 30% Faster (see below)
* 24% Less Memory (see below)
* Less complex
* Easier to read (subjective)
* Reduces number and size of dependencies
* Consistent with Rack's own specs
* Easy to run single spec file directly with ruby

Before conversion:

```
$ /usr/bin/time -l rake
Finished in 0.70517 seconds (files took 0.63812 seconds to load)
262 examples, 0 failures, 2 pending

2.56 real         1.82 user         0.72 sys
32632  maximum resident set size
```

After conversion:

```
$ /usr/bin/time -l rake
Finished in 0.499892s, 524.1129 runs/s, 1222.2632 assertions/s.

262 runs, 611 assertions, 0 failures, 0 errors, 2 skips

1.98 real         1.43 user         0.52 sys
24988  maximum resident set size
```
My testing showed non-deterministic behavior on lower versions of
CRuby.  This is expected as the CRuby garbage collector is
conservative and you can never be sure when a finalizer will
run.  It's still possible to fail on CRuby 2.7+, but it should
be reliable enough to include in the specs.
Rack 1.5 doesn't have encoding aware strings, so the previous
code failed when run on Rack 1.5.

The previous check was pointless considering the minimum Ruby
version you can execute the code with is Ruby 1.9.
Rake::TestTask loads minitest on older Ruby versions, and minitest
is in the standard library in older Ruby versions, but those are
both trivial to work around.  All specs pass with ruby 2.0/2.1
and rack 1.5.
This avoids runtime of checking Rack::RELEASE.
This is only called in one place, and all places where it is called,
params is a Hash, so just call build_nested_query directly.
Previously, it could return nil or an integer.
We aren't planning on supporting Ruby 1.8.
It's only ever called with non-arrays, since it is called with
elements from an Array#flatten call.

This is a module function, and technically public API, but the
method is very unlikely to be called directly, so I think we
can break backwards compatibility here.
Add a couple nocov markers around branches that are only taken
on old versions of Rack.
Pathname is mentioned in the documentation, but it's not used.
The documentation should generate correctly with the version of
rdoc installed by default on any supported version of Ruby.
Use a relative require to get the version.
Rack 2 depends on Ruby 2.2+.
@jeremyevans jeremyevans requested a review from ioquatix May 4, 2022 17:50
Delay loading the mock_digest_request file until it is needed.
This depends on digest auth support that is deprecated in the
rack main branch.  It will probably be removed from rack-test
at some point.

If Rack 3 is used, load only the specific parts of rack that
rack-test uses (request, mock, utils), so this doesn't set
up unnecessary autoloads.

Changes required are fairly small.  The Set-Cookie header in
the specs needs to be lower case.  To deal with the removal
of the rewindable body requirement, add a middleware to the
the specs that resets the initial rack.input in env back to
whatever it was originally. Only a single manual rewind call
was needed after that.

Fix cookie time expiration strings in the specs to use the
expected format.

To get this tested in CI, test on the rack main branch on Ruby
3.1.
rack-test is now expected to run on rack 3.
This isn't needed anymore as Rake::TestTask isn't used.
The previous spec description was wrong.  RFC 6265 section 5.1.4
shows that the default value if no path attribute is set for a
cookie is the directory of the current path, not the path itself.
Adjust the spec to test for that, including that subdirectories
of the path also work.
Without a domain attribute, an exact match is required per
RFC 6265 section 4.1.2.3.
Rack::Auth::Digest may or may not be already loaded.
This is only used in the specs.
Copy link
Member

@ioquatix ioquatix left a comment

Choose a reason for hiding this comment

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

Amazing work!

@jeremyevans jeremyevans merged commit 3c0edea into rack:main May 4, 2022
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.

None yet

2 participants