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

Use new UnitTesting github actions #1897

Merged
merged 15 commits into from
Nov 14, 2021
Merged

Use new UnitTesting github actions #1897

merged 15 commits into from
Nov 14, 2021

Conversation

rchl
Copy link
Member

@rchl rchl commented Nov 12, 2021

Not sure about coverage since I don't see an option to pass the token. But then I don't think we actually care about coverage.

@randy3k

@rchl
Copy link
Member Author

rchl commented Nov 12, 2021

Well, coverage works without the token. Token is only needed if we want to upload it and have the app make a comment. Do we want to make that work @rwols ?

@randy3k
Copy link
Contributor

randy3k commented Nov 12, 2021

Codecov token is not needed for public repo. With that said, we should have the options to specify it (in case it is a private Sublime Text package).

.github/workflows/main.yml Outdated Show resolved Hide resolved
@rwols
Copy link
Member

rwols commented Nov 12, 2021

I guess it's okay to work without coverage. It doesn't work well anyway because the coverage is not computed from code running in the worker thread from what I remember.

@randy3k
Copy link
Contributor

randy3k commented Nov 12, 2021

Looks like there is a bug in UnitTesting preventing upload: SublimeText/UnitTesting@4ba3426

@rchl
Copy link
Member Author

rchl commented Nov 12, 2021

The codecov-upload didn't seem to result in codecov comment being added here but we don't want it anyway so just removing it (and the codecov ENV).

@rchl
Copy link
Member Author

rchl commented Nov 12, 2021

I guess it's okay to work without coverage. It doesn't work well anyway because the coverage is not computed from code running in the worker thread from what I remember.

Yeah, the coverage numbers for sessions.py are pretty low:

plugin/core/sessions.py 843 745 12%

so I guess you are right.

We still have the report in the output but won't be using the upload. Thanks @randy3k for fixing anyway.

@randy3k
Copy link
Contributor

randy3k commented Nov 12, 2021

coverage.py could only trace high level threads created by threading library but not lower level threads used by ST worker.

@randy3k
Copy link
Contributor

randy3k commented Nov 12, 2021

Just pushed a workaround to detect coverage in the worker thread (inspired by nedbat/coveragepy#686).

@rchl
Copy link
Member Author

rchl commented Nov 13, 2021

It doesn't seem like the coverage has improved - https://github.com/sublimelsp/LSP/runs/4197708709?check_suite_focus=true

Assuming that I've tested it properly.
EDIT: I see, there is a new setting for that.

@randy3k
Copy link
Contributor

randy3k commented Nov 13, 2021

It doesn't seem like the coverage has improved - sublimelsp/LSP/runs/4197708709?check_suite_focus=true

Assuming that I've tested it properly. EDIT: I see, there is a new setting for that.

Um, it is because I am still not very certain about the workaround. So better to make it off first.

@rchl
Copy link
Member Author

rchl commented Nov 13, 2021

So better to make it off first.

Sure, just checking it out for fun.

@randy3k
Copy link
Contributor

randy3k commented Nov 13, 2021

Definitely a coverage increment here.

@rchl
Copy link
Member Author

rchl commented Nov 13, 2021

Now it's better:

plugin/core/sessions.py 843 482 43%

Not sure how accurate it is now but certainly better.

@randy3k
Copy link
Contributor

randy3k commented Nov 13, 2021

The codecov report is more readable.

@rchl
Copy link
Member Author

rchl commented Nov 13, 2021

Strange to me that the lines that declare a function or method (def foo(...):) and imports are always not covered.

But still could be useful to track relative changes, of course.

@randy3k
Copy link
Contributor

randy3k commented Nov 13, 2021

You have turned off reload_package_on_testing.

The code is loaded by Sublime before coverage starts. We'll need to reload the package to get those lines covered.

@rchl
Copy link
Member Author

rchl commented Nov 13, 2021

I think reloading the package makes things a bit flaky...

@randy3k
Copy link
Contributor

randy3k commented Nov 13, 2021

I think reloading the package makes things a bit flaky...

It could be. However, I think the Windows failure is kind of random error.

@rchl
Copy link
Member Author

rchl commented Nov 13, 2021

It could be. However, I think the Windows failure is kind of random error.

but then I've re-run and mac failed

@randy3k
Copy link
Contributor

randy3k commented Nov 13, 2021

It could be the new workaround for worker thread. It will definitely make things slower.

@rchl rchl merged commit 3fc2f21 into main Nov 14, 2021
@rchl rchl deleted the fix/use-ci-actions branch November 14, 2021 11:04
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.

3 participants