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

Add server/resetCache and use it in the server tests #11482

Merged
merged 14 commits into from Feb 5, 2024

Conversation

Simn
Copy link
Member

@Simn Simn commented Jan 11, 2024

I would like to do this because

  1. it should be faster and
  2. any problems that pop up here suggest that we're not resetting some state.

There are some failing test and I had to comment some of them out too because they raised exceptions. I don't know how people debug node.js applications without becoming alcoholics.

@skial skial mentioned this pull request Jan 11, 2024
1 task
@Simn
Copy link
Member Author

Simn commented Jan 16, 2024

Observation: The failing tests are all in ServerTests:

assertations: 808
successes: 799
errors: 0
failures: 9
warnings: 0
execution time: 36.411

results: SOME TESTS FAILURES (success: false)
cases.ServerTests
  testDiagnosticsRecache2: FAILURE .F
    line: 251, expected true
  testDiagnosticsRecache3: FAILURE ..F
    line: 265, expected true
  testSyntaxCache2: FAILURE F
    line: 307, No such completion
  testSyntaxCache: FAILURE FFF.
    line: 276, No such completion
    line: 280, No such completion
    line: 289, No such completion
  testXRedefinedFromX_2: FAILURE FFF.
    line: 479, expected true
    line: 481, expected true
    line: 483, expected true

But running with -D UTEST_PATTERN=ServerTests:

assertations: 112
successes: 112
errors: 0
failures: 0
warnings: 0
execution time: 9.794

results: ALL TESTS OK (success: true)

This means that some other test messes up some shared state.

@Simn
Copy link
Member Author

Simn commented Jan 25, 2024

I just noticed let fake_modules = Hashtbl.create 0 in typecore.ml which makes me realize that I didn't check for any global Hashtbl things.

@Simn
Copy link
Member Author

Simn commented Jan 25, 2024

Another one is Lexer.all_files, but I think that one is actually supposed to be global information because it resolves position offsets to line/column information.

... which actually makes me wonder how this is supposed to work with hxb where we don't always have any lexer information to use.

@Simn
Copy link
Member Author

Simn commented Jan 25, 2024

Merging hxb didn't change much (which is good). OCaml 5.0.0 still fails, the rest works fine now.

@kLabz I'm assigning to you because I don't know what else to do.

# Conflicts:
#	src/context/typecore.ml
# Conflicts:
#	src/compiler/server.ml
#	src/context/typecore.ml
@Simn
Copy link
Member Author

Simn commented Feb 5, 2024

So the problem here is the test for #10646. For some reason, the ContextMemory request fails on OCaml 5. That's not a problem with this PR specifically and since the changes here should be good, I'll merge and reopen #10646 to make sure we add back the test in a way that is compatible with both OCaml 4 and 5.

@Simn Simn merged commit c880d1d into development Feb 5, 2024
122 checks passed
@Simn Simn deleted the server/resetCache branch February 5, 2024 10:55
@Simn
Copy link
Member Author

Simn commented Feb 5, 2024

So speed-wise, nothing much changed after all. I thought that I tested this before and got a noticeable improvement, but that doesn't appear to be the case.

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