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

The extract_keys method may fail if the token has three or more words #172

Open
Triple-Z opened this issue Dec 8, 2020 · 5 comments
Open

Comments

@Triple-Z
Copy link

Triple-Z commented Dec 8, 2020

https://github.com/Olivine-Labs/luassert/blob/36fc3af9621696a9dfcf71c0bcd25cdbc9475cf8/src/util.lua#L322-L329

At line 328, the longkey maybe wrong in further iterations.

For example: if the tokens is {'is', 'match', 'error'}, the longkey variable will be match_match_error at the second iteration in the inner while loop. So it may fail to get the longest matching key when the key has three or more words.

Here is the potential fix which I brought up:

-      longkey = (token .. '_' .. key)
+      longkey = i > 1 and (tokens[i-1] .. '_' .. key) or key
@Tieske
Copy link
Member

Tieske commented Dec 8, 2020

a failing test case would be nice

@Tieske
Copy link
Member

Tieske commented Dec 10, 2020

tried this;

  it("multiple section modifiers #only", function()
    assert.is_not_is_not_equal(1, 1)
    assert.not_is_not_is_equal(1, 1)
    assert.not_not_is_is_equal(1, 1)
    assert.is_is_not_not_equal(1, 1)
    assert.is_not_is_not_is_not_equal(1, 2)
    assert.not_is_not_is_not_is_equal(1, 2)
    assert.not_not_not_is_is_is_equal(1, 2)
    assert.is_is_is_not_not_not_equal(1, 2)
  end)

But that passes nicely, so cannot reproduce this. Am I missing something @Triple-Z ?

@Triple-Z
Copy link
Author

Triple-Z commented Dec 10, 2020

@Tieske Sorry, I consider this issue is created by assertions but no modifiers.

Here is my test for this:

it("three words token #only", function()
  assert:register("assertion", "three_words_token", function() print("can you see me?") return false end, "assertion.same.positive", "assertion.same.negative")
  assert:register("assertion", "more_three_words_token", function() print("can you see me now?") return false end, "assertion.same.positive", "assertion.same.negative")

  assert.is_not_more_three_words_token()
end)

The luassert will raise an error: "luassert: unknown modifier/assertion: 'is_not_more" for this test.


With the change which I proposed before, luassert will recognize the longer token and match it first.

Like this test:

it("three words token #only", function()
  assert:register("assertion", "three_words_token", function() print("you can't see me") return false end, "assertion.same.positive", "assertion.same.negative")
  assert:register("assertion", "than_three_words_token", function() print("you can't see me") return false end, "assertion.same.positive", "assertion.same.negative")
  assert:register("assertion", "more_than_three_words_token", function() print("you can see me") return false end, "assertion.same.positive", "assertion.same.negative")

  assert.is_not_more_than_three_words_token()
end)

However, I find there is another bug, the luassert cannot find the longest token if the tokens are not continuous.

Like this:

it("three words token #only", function()
  assert:register("assertion", "three_words_token", function() print("you can't see me") return false end, "assertion.same.positive", "assertion.same.negative")
  -- assert:register("assertion", "than_three_words_token", function() print("you can't see me") return false end, "assertion.same.positive", "assertion.same.negative")
  assert:register("assertion", "more_than_three_words_token", function() print("you can see me") return false end, "assertion.same.positive", "assertion.same.negative")

  assert.is_not_more_than_three_words_token()
end)

It would raise an error: "luassert: unknown modifier/assertion: 'is_not_more_than'" .

@Tieske
Copy link
Member

Tieske commented Dec 26, 2020

tbh, I think the best route would be to document that tokens can have a maximum of 3 words. That is fair enough imo, and prevents us from going down a rabbit hole with this one.

wdyt @ajacksified @DorianGray ?

@redcatbear
Copy link

I just ran into the same problem, but in a little bit more surprising setup. Executing a spec with require "busted.runner"() directly worked just fine with a three-word-assertion. Running the same code from the console with busted gave me the "unknown modifier/assertion" error. Thanks to this thread I was able to shorten the assertion and circumvent the problem. Still quite the pitfall.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants