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

Fix and document tests #25

Merged
merged 14 commits into from
Aug 6, 2024
Merged

Conversation

necto
Copy link
Contributor

@necto necto commented Jul 13, 2024

After the change in how lsp-sonarlint fetches the SonarLint executable, tests started to fail on CI, because the SonarLint executable is not downloaded automatically and lsp-mode skips it with only a warning

The following servers support current file but do not have automatic installation: sonarlint

This change introduces an explicit SonarLint download step before running the tests.
I decided to make it an explicit step instead of integrating it into the test harness because it can take several minutes.

Additionally, this PR documents how you can run tests locally, and makes lsp-sonarlint-display-rule-descr-test more reliable.

@necto
Copy link
Contributor Author

necto commented Jul 13, 2024

There are 3 kinds of failures remaining:

  1. windows-latest + emacs snapshot
    This seems to be Eask problem - it crashes on the emacs (read-from-whole-string ARG) function.
    It might be that Emacs or Eask is improperly installed on Windows.
    I have no Windows machine and little Windows skill to investigate.
    I did manage to hit similar bug when I was trying to install Emacs snapshot on my linux machine with
    guix shell -K --without-tests=emacs-pgtk --with-git-url=emacs-pgtk=https://github.com/emacs-mirror/emacs --with-commit=emacs-pgtk=ce56f939affe8147f4172aa030058029a3c7922b emacs-pgtk
    

The installation command failed some tests (despite --without-tests flag), but I was able to get the Emacs 31.0.5 binary.
When I put it into PATH and tried to run eask status, I got the same error:

Can't read whole string

Note that this job was already failing before this change.
I will remove it from CI for now, and let someone with Windows capability resuscitate it when needed.

  1. MacOS/Ubuntu + Emacs snapshot
    The tests run ad infinum printing "Timeout while waiting for response. Method: shutdown".
    In a normal run, this error is printed only a couple of times per test (that's the way we shutdown SonarLint LSP server).
    It seems something is odd with the snapshot + it does not play well with GitHub Actions.
    I was able to reproduce the crazy error-repetition locally using act
    However, unlike GH Action, the tests printed a lot of copies of this same error message and then succeeded in regular time.
    When I attached to the docker container produced by act I was able to run the tests manually
    And I've seen many copies of the same error message.
    However, when ran interactively from Emacs within that container,
    I only see normal amount of these messages in the *Messages* buffer
    I will remove testing Emacs snapshot versions for now.

  2. MacOS all Emacs versions
    Here lsp-sonarlint-c++-reports-issues fails (times out). I don't know what is going on here (maybe an issue with path to compiler_commands.json?). @mjburling apparently succeeded at using C++ analyzer, so I would appreciate if you, @mjburling could run the test suite (as explained in the readme version in this PR) and report if it runs well on your machine.

@jcs090218
Copy link
Member

  1. windows-latest + emacs snapshot

You can ignore this error for now, see emacs-eask/cli#224.

# experimental: true
# - os: windows-latest
# emacs-version: snapshot
# experimental: true
Copy link
Member

Choose a reason for hiding this comment

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

We can just remove the windows-latest test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the confusion. I mean comment out only the windows snapshot.

Copy link
Contributor Author

@necto necto Jul 29, 2024

Choose a reason for hiding this comment

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

The other two are problematic as well. as you can see, they take all the time in the world (6h), even though they will eventually pass.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, okay. I guess we can just comment them all out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done 33f07a2

@@ -17,22 +17,22 @@ jobs:
strategy:
fail-fast: false
matrix:
os: [ubuntu-latest, macos-latest]
# TODO: fix lsp-sonarlint-c++-reports-issues on macos and include it
os: [ubuntu-latest]
Copy link
Member

Choose a reason for hiding this comment

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

Can we ignore specific tests instead of entirely removing the macos-latest? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, that's better. I ignored just lsp-sonarlint-c++-reports-issues on macos

@necto
Copy link
Contributor Author

necto commented Aug 1, 2024

The tests fail now due to a problem with setup-emacs GH Action. I cannot rerun them to see if the problem is fixed.

@jcs090218
Copy link
Member

LGTM. Thank you for your effort on the CI tests! :D

@jcs090218 jcs090218 merged commit ad668f7 into emacs-lsp:master Aug 6, 2024
5 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.

2 participants