Skip to content

Conversation

@st0012
Copy link
Member

@st0012 st0012 commented Jun 22, 2023

Motivation

Currently we always pop a class from the stack when we run after_class, but only push test classes when we run on_class. This means that if we encounter a non-test class in the middle of a test class, we will mistakenly pop the test class from the stack.

Implementation

This commit fixes this by pushing a non-test class to the stack too. But while doing so, we also need to check if on_def is in a test class or not.

Automated Tests

Added a new case for it.

Manual Tests

Currently, the test_after_request_hook in HoverExpectationsTest doesn't have code lens due to the issue I described. It should appear when switched to this branch.

Currently we always pop a class from the stack when we run `after_class`,
but only push a test class when we run `on_class`. This means that if we
encounter a non-test class in the middle of a test class, we will mistakenly
pop the test class from the stack.

This commit fixes this by pushing a non-test class to the stack too. But
while doing so, we also need to check if `on_def` is in a test class or not.
@st0012 st0012 added the bugfix This PR will fix an existing bug label Jun 22, 2023
@st0012 st0012 added this to the 2023-Q2 milestone Jun 22, 2023
@st0012 st0012 self-assigned this Jun 22, 2023
@st0012 st0012 requested a review from a team as a code owner June 22, 2023 21:21
@st0012 st0012 requested review from KaanOzkan and egiurleo June 22, 2023 21:21
@github-actions
Copy link
Contributor

Benchmark results in seconds (slowest at top)

          textDocument/completion average: 0.301594 std_dev: 0.006206
          textDocument/diagnostic average: 0.048505 std_dev: 0.0107
      textDocument/selectionRange average: 0.004118 std_dev: 0.000549
   textDocument/documentHighlight average: 0.002113 std_dev: 0.000153
        textDocument/documentLink average: 0.00203 std_dev: 0.000158
            textDocument/codeLens average: 0.001998 std_dev: 0.000188
 textDocument/semanticTokens/full average: 0.001927 std_dev: 0.00022
      textDocument/documentSymbol average: 0.001923 std_dev: 0.000111
        textDocument/foldingRange average: 0.001849 std_dev: 0.000127
textDocument/semanticTokens/range average: 0.000941 std_dev: 8.2e-05
               codeAction/resolve average: 0.000764 std_dev: 8.6e-05
               textDocument/hover average: 0.000707 std_dev: 8.0e-05
           textDocument/inlayHint average: 0.000697 std_dev: 4.6e-05
    textDocument/onTypeFormatting average: 0.000129 std_dev: 5.0e-05
          textDocument/formatting average: 8.1e-05 std_dev: 0.000333
          textDocument/codeAction average: 4.7e-05 std_dev: 3.3e-05


================================================================================
Comparison with main branch:

 textDocument/semanticTokens/full unchanged
textDocument/semanticTokens/range unchanged
      textDocument/documentSymbol unchanged
        textDocument/foldingRange unchanged
          textDocument/formatting unchanged
          textDocument/diagnostic unchanged
        textDocument/documentLink unchanged
           textDocument/inlayHint unchanged
      textDocument/selectionRange unchanged
   textDocument/documentHighlight unchanged
               textDocument/hover unchanged
          textDocument/codeAction unchanged
    textDocument/onTypeFormatting unchanged
               codeAction/resolve unchanged
          textDocument/completion unchanged
            textDocument/codeLens unchanged

@st0012 st0012 merged commit 3255b45 into main Jun 23, 2023
@st0012 st0012 deleted the fix-code-lens branch June 23, 2023 14:04
@shopify-shipit shopify-shipit bot temporarily deployed to production June 23, 2023 15:44 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix This PR will fix an existing bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants