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

testament spec nimout too lax #16693

Closed
timotheecour opened this issue Jan 12, 2021 · 2 comments · Fixed by #16698
Closed

testament spec nimout too lax #16693

timotheecour opened this issue Jan 12, 2021 · 2 comments · Fixed by #16698
Assignees

Comments

@timotheecour
Copy link
Member

timotheecour commented Jan 12, 2021

nimout spec seems to just check that each line in nimout is a contiguous substring of a line in compilation output. This is way too lax for being usable, as shown here:

Example 1

discard """
  nimout: '''
foobar2
foobar
foobar
foobar2
oobar
ba
'''
"""

static: echo "foobar1"
static: echo "foobar2"

XDG_CONFIG_HOME= nim r testament/testament.nim r tests/misc/t11682b.nim

Current Output

PASS: tests/misc/t11682b.nim c

Expected Output

fails

Example 2

this can go horribly wrong, see #16698 (comment)

Possible Solution

  • each line in nimout should be a complete line in compilation output (no substring match)
  • it's ok to have unmatched lines in output (useful for new warnings/hints etc)
  • order cannot change
  • each matched line should only be used once

EDIT: this is maybe the thing to fix (in testament.nim):

proc greedyOrderedSubsetLines*(lhs, rhs: string): bool =
  ## returns true if each stripped line in `lhs` appears in rhs, using a greedy matching.
  let rhs = rhs.strip
  var currentPos = 0
  for line in lhs.strip.splitLines:
    currentPos = rhs.find(line.strip, currentPos)
    if currentPos < 0:
      return false
  return true

Additional Information

@ringabout
Copy link
Member

ringabout commented Jan 12, 2021

Related

proc greedyOrderedSubsetLines*(lhs, rhs: string): bool =
  ## returns true if each stripped line in `lhs` appears in rhs, using a greedy matching.
  let rhs = rhs.strip
  var currentPos = 0
  for line in lhs.strip.splitLines:
    currentPos = rhs.find(line.strip, currentPos)
    if currentPos < 0:
      return false
  return true

@timotheecour
Copy link
Member Author

indeed, was using an outdated proc before greedyOrderedSubsetLines was introduced; I fixed issue message.

@ringabout ringabout self-assigned this Jan 12, 2021
timotheecour added a commit that referenced this issue Apr 4, 2021
Co-authored-by: Timothee Cour <timothee.cour2@gmail.com>
PMunch pushed a commit to PMunch/Nim that referenced this issue Mar 28, 2022
Co-authored-by: Timothee Cour <timothee.cour2@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants