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

Adding Playdate support to stds #70

Merged
merged 4 commits into from
Jun 22, 2022
Merged

Adding Playdate support to stds #70

merged 4 commits into from
Jun 22, 2022

Conversation

DidierMalenfant
Copy link

Following conversation #68 with @alerque, here is a first pass at adding support for the Playdate SDK as an stds option.

I have one test failing when running busted but the same test fails in the main branch too so I'm assuming that's not my fault.

I tested it against my code and the sample code found in the SDK and also by merging it with #66 which seems to be working fine.

I, for now, decided not to add any instance methods because they are mostly called via instance:methodName() which luacheck wouldn't catch (AFAIK) and thought it may also help catch incorrect calls like instance.methodName().

Let me know how this looks. Comments/criticism are welcome.

@alerque alerque added the enhancement New feature or request label Jun 17, 2022
@DidierMalenfant
Copy link
Author

Is there anything I can do on my end to investigate the unit test failures? I'm currently getting 3 errors on my PR branch:

Failure → spec/cli_spec.lua @ 70
cli allows measuring performance
spec/cli_spec.lua:71: Expected objects to be equal.
Failure → spec/cli_spec.lua @ 1330
cli config loading global path uses global path as fallback if --[no-]config is not used
spec/cli_spec.lua:1331: Expected objects to be equal.
Failure → spec/cli_spec.lua @ 1338
cli config loading global path detects errors in global path config
spec/cli_spec.lua:1339: Expected strings to match.

and 2 warnings:

    spec/samples/compat.lua:1:2: accessing undefined variable 'setfenv'
    spec/samples/compat.lua:1:22: accessing undefined variable 'setfenv'

@alerque
Copy link
Member

alerque commented Jun 17, 2022

I don't see any test failures or lint warnings at all. What/where/how are you running tests that show those?

I started to review this earlier today. I didn't get all the way through it but I think the biggest barrier is probably adjusting the commit messages to something conventional commits compatible so this shows up in the release notes correctly. I didn't see anything major that needed changing before merging.

@DidierMalenfant
Copy link
Author

I can absolutely redo the commit messages, maybe resubmit this as a new PR with just one commit?

I get the errors when I run busted from the luacheck folder.

didier@MacBook-Pro-de-Didier luacheck % busted
●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●◼●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●◼◼●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●
450 successes / 3 failures / 0 errors / 0 pending : 4.706792 seconds

Is there some kind of a local state that could affect my result versus yours?

@alerque
Copy link
Member

alerque commented Jun 17, 2022

No need need for a different PR, we can rebase this one just fine. I can even handle it with git revise on my end before merging, maybe let me finish reviewing as it is first.

@arichard4
Copy link

Didier, one question on the test failures- are you running with luajit? The luajit test runner seems to have occasional test failures when run via CI, see e.g. discussion in #46

@DidierMalenfant
Copy link
Author

are you running with luajit?

Not sure. How do I check for that?

@arichard4
Copy link

I don't know if you can get it out directly, but you can explicitly pass a lua binary to busted to use. i.e.
lua -v (to get the version)
which lua (to get the path to your lua)
busted --lua=$path (to see if you can repro with whatever lua version you have under lua)

@alerque
Copy link
Member

alerque commented Jun 21, 2022

I don't know if you can get it out directly

Sure you can, like this:

$ busted -e 'print(jit and jit.version or _VERSION)'

@DidierMalenfant
Copy link
Author

$ busted -e 'print(jit and jit.version or _VERSION)'

I get just plain Lua 5.4

@arichard4
Copy link

Failure → spec/cli_spec.lua @ 70

I can repro this, the issue is that you don't have luasocket installed. Try luarocks install luasocket, assuming you have luarocks.

Failure → spec/cli_spec.lua @ 1330
Failure → spec/cli_spec.lua @ 1338

I can't repro these, but from the context and the test names I'm wondering if you have luafilesystem installed. (When I uninstall it to test, busted itself breaks; but you may have gotten busted a different way.)


We should probably add explicit documentation for running the tests, right now the luasocket requirement in particular seems to be both undocumented and invisible, since the failing test only checks the return code.

@alerque
Copy link
Member

alerque commented Jun 22, 2022

Thanks for ferreting out that info @arichard4 that's helpful. As I understand it this PR is actually good to be merged and those failures are all local to Didier's testing setup. Tests pass in CI and for me locally and I don't see any other reason to hold up merging this. Am I missing anything?

@arichard4
Copy link

Nope, this PR looks good to me.

@alerque
Copy link
Member

alerque commented Jun 22, 2022

I revised the Git commit messages to be conventional commits compliant, but otherwise no change...

Thanks for the contribution @DidierMalenfant!

@alerque alerque enabled auto-merge June 22, 2022 08:25
@alerque alerque merged commit 5d833a2 into lunarmodules:master Jun 22, 2022
@DidierMalenfant DidierMalenfant deleted the playdate-supprt branch June 22, 2022 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

3 participants