glacode / yamma

VSCode extension for Metamath
10 stars 2 forks source link

Fix the tests so they run (not actually pass, but at least they run) #2

Closed Antony74 closed 1 year ago

Antony74 commented 1 year ago

npm audit is showing some 'vunerabilities'. This PR would address them.

Looks like all your unit testing is done with jest, which is good, so I've removed mocha.

Looks like the latest eslint has a new default rule concerning semi-colons, to see the new lint failures:

npm run lint

To get rid of them

npm run lint -- --fix

I haven't run that for you because there's a chance it might conflict with whatever you're doing at the moment. Alternatively the semi-colon rule can be switched off it you don't like it.

Everything's still working, best I can tell, and these changes are pretty safe, but you'll have a better idea than me how to exercise the full functionality to make sure.

glacode commented 1 year ago

Thank you, Antony

I'll see what I can find locally with npm audit (and I hope I can learn something...) and compare it with your PR

I'm using Jest, but it looks like the Integration test "generated" by the LSP sample (that everybody starts from) is using Mocha (and I've not been able to run it, but it's just because of my poor knowledge of this ecosystem)

Glauco

Antony74 commented 1 year ago

Experimenting with npm audit locally sounds like a good plan to me.

I don't know how to recover the integration test you mention. There's no 'scripts' directory in the yamma repo, and the latest LSP sample doesn't seem to contain integration test generation or the mocha dependancy either

https://github.com/microsoft/vscode-extension-samples/blob/main/lsp-embedded-language-service/package.json

glacode commented 1 year ago

Here

https://github.com/glacode/yamma/blob/master/.vscode/launch.json

"Language Server E2E Test"

is meant to be the integration test (from what I understand)

If I remember correctly, the mocha dependency was on the client package, and trying to fix things, I ended up adding the dependency in the server package, and then I got conflicts with jest, that I finally sort out.

Please, let me know if this makes any sens... :-)

Antony74 commented 1 year ago

Thanks. I believe I've sucessfully restored the tests. They don't actually pass, but I think that's down to the yamma extension not doing the things the lsp-sample extension does. i.e. this is expected, and we'd either need to write different client side tests, or abandon this testing strategy.

glacode commented 1 year ago

It's a good chance for me to learn some npm stuff.

I'm trying to understand all your changes and to recreate them locally.

I'm curious to see the E2E test behavior (and see if we can have it working)

glacode commented 1 year ago

Tomorrow I will work on the eslint update, to see how close I can get to your new package.json

Antony74 commented 1 year ago

Well done, you've fixed npm audit. I'm not sure how because you haven't touched all the affected package.json dependencies, but it's passing now. Actually if you deleted the package-lock.json files and allowed them to regerate, latching themselves to later permitted package version, that might have done it. Otherwise, it's a complete mystery. But if npm audit is happy, then I'm happy.

Anyway, I've renamed this PR to reflect what's left in it.

Now the lsp-sample adds some simple demo functionality to .txt files.

This is functionality that yamma does not provide, and it is correct in not doing so. Therefore once the tests are actually running, then they will fail, which is also correct because they are tests for lsp-sample not for yamma.

Similar tests could be written for yamma with .mm and .mmp fixtures, but only if you wanted them. Personally I'm more interested in your server code - initially parsers.

Antony74 commented 1 year ago

By the way, when I try to run lsp-sample and manually play with the functionality I've described above, nothing actually happens, but if I run the e2e tests, then they pass, and I can see evidence of uppercase words getting flagged as warnings.

glacode commented 1 year ago

Thank you, Antony.

I hope the electron based test, will save us some "visual/manual" testing, in the future.

Now, even though there are hundreds of Jest tests, I'm always going to have an actual look at the behavior.

And, anyway, it seems a nice skill to have.

Glauco

glacode commented 1 year ago

Actually if you deleted the package-lock.json files and allowed them to regerate, latching themselves to later permitted package version, that might have done it. Otherwise, it's a complete mystery. But if npm audit is happy, then I'm happy.

At some point I did exactly that: I deleted package-lock.json, I deleted node_modules/* and then npm install

Antony74 commented 1 year ago

Error: Cannot find module 'mocha'

Sorry, we need to add that mocha dependency back in. (you might not see this error when you npm run test because mocha's still sitting around in your node_modules directory, but delete the directory and install again and you'll see it).

npm install -D mocha

Antony74 commented 1 year ago

And, anyway, it seems a nice skill to have.

Getting some experience of an e2e framework is a good idea. I've no idea which e2e framework would be most marketable, though probably not this one :-)

Some developers will treat full unit test coverage like it's "proof" of the program's correctness. Of course it's nothing of the sort ;-)