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

[Refractor] Refractor lsp unit test #1379

Merged
merged 4 commits into from
May 31, 2024
Merged

[Refractor] Refractor lsp unit test #1379

merged 4 commits into from
May 31, 2024

Conversation

Wck-iipi
Copy link
Contributor

1. Does this PR affect any open issues?(Y/N) and add issue references (e.g. "fix #123", "re #123".):

2. What is the scope of this PR (e.g. component or file name):

/kclvm/tools/src/LSP/

3. Provide a description of the PR(e.g. more details, effects, motivations or doc link):

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Other

4. Are there any breaking changes?(Y/N) and describe the breaking changes(e.g. more details, motivations or doc link):

  • N
  • Y

5. Are there test cases for these changes?(Y/N) select and add more details, references or doc links:

  • Unit test
  • Integration test
  • Benchmark (add benchmark stats below)
  • Manual test (add detailed scripts or steps below)
  • Other

@Wck-iipi Wck-iipi marked this pull request as draft May 29, 2024 13:55
@Wck-iipi
Copy link
Contributor Author

Wck-iipi commented May 29, 2024

@Peefy @He1pa

  1. I have just refractored one function, are these the required changes you are looking for? If yes, I would do this to all of them.
  2. Regarding goto_def_test_snapshot macro, wouldn't it be better to check the state like start, end and file path (like here) rather than the entire object? I am saying this as this is giving the following which will be impossible to test against.
image Or do you have something else in mind regarding this snapshot.

@He1pa
Copy link
Contributor

He1pa commented May 30, 2024

@Peefy @He1pa

  1. I have just refractored one function, are these the required changes you are looking for? If yes, I would do this to all of them.
  2. Regarding goto_def_test_snapshot macro, wouldn't it be better to check the state like start, end and file path (like here) rather than the entire object? I am saying this as this is giving the following which will be impossible to test against.

image Or do you have something else in mind regarding this snapshot.

Yes, that's exactly what I want.
For the content of snapshot, I just provide an example. You can design the format by yourself. As long as it contains the information needed for the test (scalar/array/link, file, pos, etc.).

@He1pa
Copy link
Contributor

He1pa commented May 30, 2024

@Peefy @He1pa

  1. I have just refractored one function, are these the required changes you are looking for? If yes, I would do this to all of them.
  2. Regarding goto_def_test_snapshot macro, wouldn't it be better to check the state like start, end and file path (like here) rather than the entire object? I am saying this as this is giving the following which will be impossible to test against.

image Or do you have something else in mind regarding this snapshot.

You need to pay attention to the absolute path of the snapshot path, which will cause CI to fail. I provide a simple version in #1381, you can optimize it

Copy link
Contributor

@Peefy Peefy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Wck-iipi Wck-iipi marked this pull request as ready for review May 31, 2024 09:58
@Peefy Peefy merged commit c07925f into kcl-lang:main May 31, 2024
7 of 11 checks passed
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.

[Refactor] refactor lsp unit test
3 participants