Skip to content

Refactor rstests as tests#1080

Open
ymcx wants to merge 8 commits into
nushell:mainfrom
ymcx:refactor-rstests-as-tests
Open

Refactor rstests as tests#1080
ymcx wants to merge 8 commits into
nushell:mainfrom
ymcx:refactor-rstests-as-tests

Conversation

@ymcx
Copy link
Copy Markdown

@ymcx ymcx commented May 16, 2026

As (shortly) discussed in #1078 and first introduced in #704 (comment).

Just like the title implies, this PR pretty much replaces all rstest unit tests along with their test cases with regular tests by manually iterating over all of the individual test cases one at a time.

Even though the refactor itself is quite simple and could easily have been and probably should have been automated, I did it completely by hand. I tried to keep all ignored test cases intact along with the comments if any, but I can't guarantee I didn't miss any. Other than that it should functionally match 1:1 to the previous behaviour.

I'm sorry for whoever has to go through the changelog as it's quite unreadable due to the sheer size of it.

@kronberger-droid
Copy link
Copy Markdown
Collaborator

I will give it a shot if i find some time.

@sholderbach
Copy link
Copy Markdown
Member

sholderbach commented May 26, 2026

That is certainly a tedious change to roll by hand. Going ahead with it should certainly current benefits into account instead of just the opinion of someone who doesn't spend their time anymore on this repo.
But my two cents regarding rstest are, that there is a significant benefit during debugging if you have those case matrices separated into their own test function to run. Thus they can run in parallel and fail and succeed individually, compared to loops of tests that fail at first assertion. With that you can share the same test logic and quickly drill down which invariant or property is broken instead of having to recompile and run until all tests in the chain pass. Nevertheless there are the additional proc-macro funs, so worth weighing what is important now.

On obvious downside is that the syntax for the case macros isn't the prettiest (but IMHO better than some of the pytest idiosyncracies that came from the python inspiration)

@kronberger-droid
Copy link
Copy Markdown
Collaborator

kronberger-droid commented May 26, 2026

I have been investigating the root for the rtests change: it traces back to @stormasm in #704, for two reasons (1) it seemed to be blocking nextest and (2) he couldn't figure out how to #[ignore] individual cases.

Both look moot now. The nextest problem was traced to tests touching the terminal , not rtest and per-case ignore works fine now.

So i would agree with @sholderbach's objections, no parallel running of tests, no ignored in test output, no per-case pass/fail and cargo test name::case filtering.

I am very sorry @ymcx since you definitely put a lot of work into this PR, but I would lean towards keeping rtest.
Please correct me if I have understood something wrong and there was another reason to remove it. (#1078 just for rev)

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