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

WIP: Add snapshot tests for visual samples #1939

Closed
wants to merge 3 commits into from

Conversation

nsfisis
Copy link
Contributor

@nsfisis nsfisis commented Apr 5, 2023

Summary

This PR adds snapshot tests for visual samples (spec/visual/samples/*). It helps contributors to check if their changes cause regressions or not.

Examples

Run rake spec for my PR #1938:

  5) Failure:
Rouge::Lexers::Rouge::Lexers::JSX#test_0003_lexes the sample without changes [/home/ken/src/fork/rouge/spec/lexers_spec.rb:47]:
--- expected
+++ actual
@@ -424,7 +424,7 @@
 Text:  \" \"
 Punctuation:  \"{\"
 Text:  \"<newline>    \"
-Name.Function:  \"super\"
+Keyword:  \"super\"
 Punctuation:  \"();\"
 Text:  \"<newline>    \"
 Keyword:  \"this\"
@@ -513,7 +513,7 @@
 Literal.String.Delimiter:  \"'\"
 Punctuation:  \";\"
 Text:  \"<newline>    \"
-Name.Function:  \"return \"
+Keyword:  \"return \"
 Punctuation:  \"(\"
 Text:  \"<newline>      \"
 Punctuation:  \"<\"

The failure log shows that the PR changes token types of super and return in JavaScript.

How to work

  • If a snapshot file does not exist, a new snapshot is taken and saved. If a snapshot file is found, it is compared to the actual value.
  • If the UPDATE_SNAPSHOTS environment variable is defined, all snapshots will be updated.
  • Snapshot files are saved in spec/__snapshots__/{language}.snap.

TODO

  • Write a guide for contributors about how to update snapshots.
  • Fix potentially flaky tests.
    • Some lexers seem to return slightly different results for each test run.

@nsfisis
Copy link
Contributor Author

nsfisis commented Apr 6, 2023

  • Fix potentially flaky tests.
    • Some lexers seem to return slightly different results for each test run.

The snapshot test of Twig sometimes fails.

It is because Twig lexer shares @@keywords variable with its parent class, Jinja.
Both class defines keywords method like this:

class Jinja
  def self.keywords
    @@keywords ||= ...
  end
end

class Twig < Jinja
  def self.keywords
    @@keywords ||= ...
  end
end

If Twig.keywords is called before Jinja.keywords, @@keywords is set to Twig's and both class returns the same value. If not, @@keywords is initialized to Jinja's.
The snapshot test of Twig fails only if the test of Twig lexer is run after Jinja is.

@nsfisis
Copy link
Contributor Author

nsfisis commented May 3, 2023

  • Fix potentially flaky tests.
    • Some lexers seem to return slightly different results for each test run.

I found another. The snapshot test of Wollok sometimes fails.

It is because Wollok lexer has a class-scoped local variable, entities, to store entity list, which the lexer distinguishes token types by. Class-scoped local variables are shared between its instances in Ruby. As a result, a token type sometimes change depending on the state of entities.

@jneen
Copy link
Member

jneen commented May 3, 2023

Good catch. Double-@ class variables shouldn't be used in lexers.

@nsfisis
Copy link
Contributor Author

nsfisis commented Dec 2, 2023

I am closing this PR as it would break the CI tests for pending or active PRs, requiring the authors of those PRs to address the CI failures.

@nsfisis nsfisis closed this Dec 2, 2023
@nsfisis nsfisis deleted the snapshot-tests branch December 2, 2023 02:14
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.

None yet

2 participants