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

DeltaTest improvements #876

Merged
merged 2 commits into from
Jan 4, 2022
Merged

DeltaTest improvements #876

merged 2 commits into from
Jan 4, 2022

Conversation

WayneD
Copy link
Contributor

@WayneD WayneD commented Jan 2, 2022

  • Added .expect_contains(), .expect_raw_contains(), and .expect_contains_once() member functions to DeltaTestOutput. These functions also output some helpful debug info when the assert fails.
  • Separated .with_config_and_input() into .with_config() and (the tweaked) .with_input().
  • Made .with_config() create a DeltaTest object (like .with() does).
  • Renamed .with() as .with_args().
  • Moved .explain_ansi() from DeltaTestOutput to DeltaTest, which must now be called prior to .with_input() since the latter stashes off both the raw_output & the processed output in the object.
  • Changed .expect() and .expect_skip() to return Self so that they can continue a chain of multiple expect calls (e.g. a series of partial matches with different skip values).
  • The processed output text can be accessed via test_obj.output (see also test_obj.raw_output).
  • Renamed .expect_skip() to .expect_after_skip().
  • Changed .expect() to start at the first line.
  • Added .expect_after_header() that works like the old .expect().
  • Renamed lines_match() to assert_lines_match() and made it match all lines (no skip number).
  • Added assert_lines_match_after_skip() to work like the old lines_match() function, but with the .expect_after_skip() arg order.
  • Converted some old-style tests into DeltaTest style tests.

@dandavison
Copy link
Owner

cc @th1000s who's the creator of DeltaTest.

@WayneD WayneD force-pushed the delta-test branch 5 times, most recently from 3565f45 to eab7048 Compare January 2, 2022 18:57
- Added .expect_contains(), .expect_raw_contains(), and
  .expect_contains_once() member functions to DeltaTestOutput.
  These functions also output some helpful debug info when the
  assert fails.
- Separated .with_config_and_input() into .with_config() and (the
  tweaked) .with_input().
- Made .with_config() create a DeltaTest object (like .with() does).
- Renamed .with() as .with_args().
- Moved .explain_ansi() from DeltaTestOutput to DeltaTest, which must
  now be called prior to .with_input() since the latter stashes off both
  the raw_output & the processed output in the object.
- Changed .expect() and .expect_skip() to return Self so that they can
  continue a chain of multiple expect calls (e.g. a series of partial
  matches with different skip values).
- The processed output text can be accessed via `test_obj.output` (see
  also `test_obj.raw_output`).
- Renamed .expect_skip() to .expect_after_skip().
- Changed .expect() to start at the first line.
- Added .expect_after_header() that works like the old .expect().
- Renamed lines_match() to assert_lines_match() and made it match all
  lines (no skip number).
- Added assert_lines_match_after_skip() to work like the old
  lines_match() function, but with the .expect_after_skip() arg order.
- Converted some old-style tests into DeltaTest style tests.
@th1000s
Copy link
Collaborator

th1000s commented Jan 3, 2022

If calling with_config(config: config::Config) using the same config multiple times happens more often in the future then storing a Cow<'a, &Config> might help as well. Which is what you have been reimplementing manually anyhow :)

tweaking someone else's code.

Oh please do, nothing helps more than someone who didn't write the code noticing that the names or logic are not as clear as they could be. Maybe also rename set_cfg to set_config?

@dandavison
Copy link
Owner

This PR is great IMO Wayne, and I'm glad to hear that Thomas approves.

Maybe also rename set_cfg to set_config

That SGTM. Do you want to make that final change and then I'll merge this?

@dandavison dandavison merged commit 1d6f18a into dandavison:master Jan 4, 2022
@dandavison
Copy link
Owner

dandavison commented Jan 4, 2022

Thanks again for this Wayne.

If calling with_config(config: config::Config) using the same config multiple times happens more often in the future then storing a Cow<'a, &Config> might help as well.

My first thought was to try to use Cow, but there were errors: our Config is not clone-able, and yet my understanding is that Cow needs to be able to create an owned version of a data structure from a reference. I tentatively concluded that what we needed was not exactly Cow but an even simpler enum over owned|reference which supports borrowing, but I didn't think it through / investigate properly.

I do see you've written &Config there, whereas what I tried was Cow<'a, Config> I think.

@th1000s
Copy link
Collaborator

th1000s commented Jan 4, 2022

Indeed, Cow<&Config> is nonsense. Making Config cloneable required a few derives and a hack for GitConfig, see #884

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.

3 participants