-
Notifications
You must be signed in to change notification settings - Fork 24
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
test: add integration testing #357
Conversation
TODO:
|
Seems to be entirely dependent on the existence of |
Got this far with GitHub action
But currently blocked by
|
Does the internet say anything about that? You would think it would be possible to run on GitHub CI since VSCode and GitHub are both Microsoft. |
Maybe this is useful: https://github.com/microsoft/vscode-test/blob/master/sample/azure-pipelines.yml |
This was the one I followed, but at least when running locally with act, it needs a lot of extra installations. The installations also take quite a lot of time each run, so either way, it might also be preferable to create a Docker image to run on. |
Do you want to wrap this PR up and then we can try GitHub CI in a new PR? I think that would be best. |
Ill give some feedback on the code. |
test/hover.test.js
Outdated
]) | ||
assert.strictEqual(r[0].contents[0].value, '\n```flix\nType\n```\n') | ||
}) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think testing the HoverProvider is a good start, if we have like 5-10 tests.
Then I think we should test inline diagnostics on a separate broken program.
But what I would also like is to check that:
- Adding a Flix file works.
- Removing a Flix file works.
- A project with a Flix toml and some dependencies works.
Essentially I am more worried about the "stuff around" than the providers, because if the HoverProvider works probably all the other providers work. What could break is the integration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should just treat this PR as an introduction of the setup, then we can write some actual test cases in another one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok,, we can wait with the file manipulation stuff, but lets get the rest done in this PR.
Lets let it be part of the test! |
It should be working as is, so sounds good 👍 |
Roger. So if you cleanup my comments then we merge it in. Then in a subsequent PR we can fiddle with the GitHub CI part. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also get a Readme about how to run the tests?
Yes please. @sockmaster27 Can you add it to |
Can't figure out what causes this but it seems harmless |
Merged! What are the next steps? |
Fixes #341