Replace `ignore` with `ignore-if`. #120

ltratt · 2024-01-30T09:35:00Z

This PR's main aim is to remove ignore and add ignore-if: <cmd> which allows a test to flexibly specify when it should/shouldn't be ignored (9ffa2b2). However, this means that the ability to specify a human string for why a test is ignored is lost so this PR also adds the ability to allow comment lines (d09c0dc).

My guess is that the main way this will be used is with test so you might have things like ignore-if: test $YKB_TRACER == "swt" or ignore-if: test $YKB_TRACER != "swt" and so on. @Pavel-Durov does this allow you to specify tests in your yk PR in the way you need?

This is a breaking change, so if we do think this is useful and fits in with lang_tester's design, then it does imply a breaking release.

This was previously slightly inconsistent in its use of punctuation (etc.). While here, clarify one slightly clunky phrasing.

Pavel-Durov · 2024-01-30T10:19:59Z

@ltratt that's perfect for my use case in YK PR!

vext01 · 2024-01-30T10:37:11Z

examples/rust_lang_tester/lang_tests/ignore.rs

@@ -1,4 +1,4 @@
-// ignore: this test is intentionally ignored


So do we lose the ability to say why a test is ignored then?

I think forcing a reason was a good thing. I can barely remember why I did things last week, let alone why someone else did something 2 years ago :)

See d09c0dc -- this PR makes things much more flexible!

I see, a later commit adds a comment token.

Hrm. I wonder if there's a way we can force the user to provide a reason. Lazy devs will not add one.

That's a human/per-project problem, not a lang_tester problem :)

I'd argue that your change made it a human/per-project problem. Before you had to provide a reason. No ifs, not buts.

Perhaps if you use ignore-if, you have to also provide a ignore-reason line too?

ignore: <reason> always had <reason> as optional, so this PR doesn't change the optionality. I don't want to mandate how people use this: indeed, the comment syntax is far more flexible than what we had before IMHO.

I didn't realise that it was optional, so I'll back down.

vext01 · 2024-01-30T10:38:06Z

src/lib.rs

-//!   * `ignore: [<string>]` specifies that this file should be ignored for the reason set out in
-//!     `<string>` (if any). Note that `<string>` is purely for user information and has no effect
-//!     on the running of tests.
+//!   * `ignore-if-cmd: <cmd>` defines a shell command that will be run to determine whether to


Should ignore-if-cmd be ignore-if or are they two separate facilities?

Oops, that's a typo from an earlier design. Fixed in 679ac45.

vext01 · 2024-01-30T10:45:47Z

Based on my preliminary look, we've added something very general: a shell command.

My worry is that forking will slow down the test suite.

For now, we only need the ability to ignore something based on environment variables.

I wonder if ignore-if should be ignore-if-env, e.g.:

// ignore-if-env: USER=edd

If you needed to skip tests based on the output of shell commands (will we need it? I don't know), it would be doable, but more effort. You'd have to get the test runner to run the shell command and set an env var.

That then raises the question: should there be ignore-if-env and ignore-if-cmd?

(I bet that was your earlier design?)

ltratt · 2024-01-30T10:47:49Z

I don't think ignore-if-env is general enough because you can't specify things like "ignore this test with opt-level > 1". I almost added an expression parser in but... given that lang_tester is already running your program as a sub-command, firing a shell with test is going to be a manageable cost IMHO.

vext01 · 2024-01-30T11:04:22Z

I don't think ignore-if-env is general enough because you can't specify things like "ignore this test with opt-level > 1".

If knowing the opt level requires a shell-out, you could do it by getting your runner (e.g. langtest_c.rs) to shell out and set an OPT_LEVEL env var based on the result. It's more work for the user, but the one shell-out can be shared for all tests.

given that lang_tester is already running your program as a sub-command, firing a shell with test is going to be a manageable cost IMHO.

I could buy that, so if you still agree that the current design is acceptable, I'll do a deeper review. Let me know.

ltratt · 2024-01-30T11:07:15Z

I started with ignore-if-env and ended up with this PR so I'm more a fan of this style, so if you buy the argument, please go ahead with a review!

vext01 · 2024-01-30T11:19:01Z

I think we should add a test that uses a ignore-if that is more than a simple true/false to check that arguments are passed to the shell properly.

ltratt · 2024-01-30T11:20:15Z

I think we should add a test that uses a ignore-if that is more than a simple true/false to check that arguments are passed to the shell properly.

Any thoughts on what?

vext01 · 2024-01-30T11:25:04Z

src/tester.rs

@@ -128,6 +130,25 @@ impl LangTester {
        self
    }

+    /// If set, defines what top-level lines will be treated as comments if they begin with


top-level is confusing me a bit here.

If you look at a typical lang_test, it looks like:

// Compiler: // stdout: // match me!

And now we are adding the ability to have an inner comment, so a test can look like:

// Compiler: // stdout: // # flibble // match me!

So when you say "defines what top-level lines will be treated as comments", I'm thinking of the top-level // comments, not the `#' ones.

Do you see my concern?

If you can think of a better phrasing, I'm all ears, but I think this comes under the heading of "this is how lang_tester works and you can't avoid knowing a bit about how a) your language parses comments b) your runner extracts test c) how lang_tester parses things".

I've just realised something else. By top-level, you mean it must appear at the same level as (e.g.) Compiler:.

So in fact my example above is invalid. But should it be? I've often wanted to comment why I'm matching something, so perhaps comments should be allowed anywhere, and then all of this "top-level" talk disappears?

We could allow comments elsewhere too, but I worry a bit that will create "comments" by accident. Maybe I'm worrying too much?

It's true that you might accidentally comment something you want to match.

I'll leave the decision up to you. I'm aware that I've been pedantic/annoying enough for one PR!

On reflection I agree that generalising this is probably a good idea. Try 2223188.

vext01 · 2024-01-30T11:26:53Z

Any thoughts on what?

Huh. I didn't ask for any thoughts :P

vext01 · 2024-01-30T13:15:51Z

src/tester.rs

-    ///     # A literal string beginning with `#`
-    /// ```
+    /// If set, defines what lines will be treated as comments if, ignoring the current level of
+    /// indentation, they begin with `comment_prefix`.


This all LGTM. I wonder if here we should say:

If set, defines what lines of a lang_tester test will be treated as comments if, ignoring the current level of indentation, they begin with comment_prefix.

Note that this is not to be confused with another language's comment lines in a test file when extracting the lang_tester test (e.g. using [LangTester::take_while].

Second sentence is iffy, but I hope you can see what I'm aiming at?

I see what you're aiming at, but I don't think it's a good idea, in part because you don't have to embed lang_tester stuff in comments.

Indeed. It's difficult to put into words.

Maybe we should just move on and wait for a user to squeal?

Maybe we should just move on and wait for a user to squeal?

I tend to agree.

vext01 · 2024-01-30T14:45:53Z

I think it's just this test outstanding now?

ltratt · 2024-01-30T14:50:47Z

I think it's just #120 (comment) outstanding now?

I'm still awaiting an idea of what one could actually do here from you! The problem is that one is not in control of the environment, so it's hard to know what (say) an environment variable that's always set (or not) might be. I was astonished recently to find situations where even $SHELL isn't set on some systems! For example, I wonder if $PATH is set on Windows or not?

vext01 · 2024-01-30T14:56:50Z

I don't mean to test environment variables. I mean to test some shell command with more than a single argument, e.g.:

ignore-if: echo "123" | grep 2

This should skip the test, as grep exits 0 on a match.

(I want to check that echo "123" | grep 2 isn't interpreted as a single argument with spaces inside)

(obviously guard such a test for unix platforms)

ltratt · 2024-01-30T15:09:28Z

(obviously guard such a test for unix platforms)

How?

ltratt · 2024-01-30T15:18:00Z

@vext01 Actually, I can't be bothered guarding for Unix, so d45f9aa adds tests (and fixes an issue where ignore-if splats on the terminal) that are Unix only --- but I suspect even Windows might pass this test these days!

vext01 · 2024-01-30T15:19:41Z

Great! thanks.

And sorry (not sorry) for being persistent :)

Please squash.

`ignore` is a blunt tool. `ignore-if` allows users to be selective if/when they ignore tests. `ignore-if: true` is equivalent to the old `ignore`.

ltratt · 2024-01-30T15:24:54Z

Squashed.

Homognise text.

5b62645

This was previously slightly inconsistent in its use of punctuation (etc.). While here, clarify one slightly clunky phrasing.

ltratt assigned vext01 and Pavel-Durov Jan 30, 2024

vext01 reviewed Jan 30, 2024

View reviewed changes

ltratt added 2 commits January 30, 2024 15:24

Replace ignore with ignore-if.

7df5cd0

`ignore` is a blunt tool. `ignore-if` allows users to be selective if/when they ignore tests. `ignore-if: true` is equivalent to the old `ignore`.

Allow users to specify a literal string that defines comments in tests.

2a94d9d

ltratt force-pushed the ignore_if branch from d45f9aa to 2a94d9d Compare January 30, 2024 15:24

vext01 added this pull request to the merge queue Jan 30, 2024

Merged via the queue into softdevteam:master with commit a759449 Jan 30, 2024
2 checks passed

ltratt deleted the ignore_if branch July 18, 2024 14:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `ignore` with `ignore-if`. #120

Replace `ignore` with `ignore-if`. #120

ltratt commented Jan 30, 2024

Pavel-Durov commented Jan 30, 2024

vext01 Jan 30, 2024

ltratt Jan 30, 2024

vext01 Jan 30, 2024

ltratt Jan 30, 2024

vext01 Jan 30, 2024

ltratt Jan 30, 2024

vext01 Jan 30, 2024

vext01 Jan 30, 2024

ltratt Jan 30, 2024

vext01 commented Jan 30, 2024

ltratt commented Jan 30, 2024 •

edited

Loading

vext01 commented Jan 30, 2024 •

edited

Loading

ltratt commented Jan 30, 2024

vext01 commented Jan 30, 2024

ltratt commented Jan 30, 2024

vext01 Jan 30, 2024

ltratt Jan 30, 2024

vext01 Jan 30, 2024

ltratt Jan 30, 2024

vext01 Jan 30, 2024

ltratt Jan 30, 2024

vext01 commented Jan 30, 2024

vext01 Jan 30, 2024 •

edited

Loading

ltratt Jan 30, 2024

vext01 Jan 30, 2024

ltratt Jan 30, 2024

vext01 commented Jan 30, 2024

ltratt commented Jan 30, 2024

vext01 commented Jan 30, 2024

ltratt commented Jan 30, 2024

ltratt commented Jan 30, 2024

vext01 commented Jan 30, 2024

ltratt commented Jan 30, 2024

		@@ -1,4 +1,4 @@
		// ignore: this test is intentionally ignored

Replace ignore with ignore-if. #120

Replace ignore with ignore-if. #120

Conversation

ltratt commented Jan 30, 2024

Pavel-Durov commented Jan 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vext01 commented Jan 30, 2024

ltratt commented Jan 30, 2024 • edited Loading

vext01 commented Jan 30, 2024 • edited Loading

ltratt commented Jan 30, 2024

vext01 commented Jan 30, 2024

ltratt commented Jan 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vext01 commented Jan 30, 2024

vext01 Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vext01 commented Jan 30, 2024

ltratt commented Jan 30, 2024

vext01 commented Jan 30, 2024

ltratt commented Jan 30, 2024

ltratt commented Jan 30, 2024

vext01 commented Jan 30, 2024

ltratt commented Jan 30, 2024

Replace `ignore` with `ignore-if`. #120

Replace `ignore` with `ignore-if`. #120

ltratt commented Jan 30, 2024 •

edited

Loading

vext01 commented Jan 30, 2024 •

edited

Loading

vext01 Jan 30, 2024 •

edited

Loading