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

ci: build and push to cachix on merge #47

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

mrcjkb
Copy link
Member

@mrcjkb mrcjkb commented Nov 10, 2024

No description provided.

@mrcjkb mrcjkb marked this pull request as draft November 10, 2024 16:28
@mrcjkb mrcjkb marked this pull request as ready for review November 11, 2024 18:30
@mrcjkb
Copy link
Member Author

mrcjkb commented Nov 11, 2024

🤔 I don't understand how the checks are failing

@mrcjkb mrcjkb marked this pull request as draft November 11, 2024 18:30
@mrcjkb mrcjkb marked this pull request as ready for review November 11, 2024 18:34
@mrcjkb
Copy link
Member Author

mrcjkb commented Nov 11, 2024

@simifalaye the new parse_spec cases, "Can set entire table" and "Can add entire table" appear to be flaky. Any idea why?

Here, the same checks have passed for the pull_request trigger, but failed for the push trigger.

I can reproduce the flakiness in the nix devshell, by running

mkdir luarocks
luarocks make --tree luarocks --deps-mode-all
busted # run this repeatedly
       > ●●●●●●●●●●●●◼◼●●●●●●●
       > 19 successes / 2 failures / 0 errors / 0 pending : 0.01698 seconds
       >
       > Failure → spec/parse_spec.lua @ 111
       > parse Can set entire table
       > spec/parse_spec.lua:129: Expected objects to be equal.
       > Passed in:
       > (string) '[rocks]
       >
       > [rocks.neorg]
       > pin = true
       > version = "2.0.0"
       > '
       > Expected:
       > (string) '[rocks]
       >
       > [rocks.neorg]
       > version = "2.0.0"
       > pin = true
       > '
       >
       > stack traceback:
       >    spec/parse_spec.lua:129: in function <spec/parse_spec.lua:111>
       >
       >
       > Failure → spec/parse_spec.lua @ 131
       > parse Can add entire table
       > spec/parse_spec.lua:143: Expected objects to be equal.
       > Passed in:
       > (string) '[rocks]
       >
       > [rocks.neorg]
       > pin = true
       > version = "2.0.0"
       > '
       > Expected:
       > (string) '[rocks]
       >
       > [rocks.neorg]
       > version = "2.0.0"
       > pin = true
       > '
       >
       > stack traceback:
       >  spec/parse_spec.lua:143: in function <spec/parse_spec.lua:131>
       >
       >
       > Error: test suite failed.
       For full logs, run 'nix log /nix/store/z7fb1pa3gbd7h7n4n1bvdh0pd7r7zhzl-luajit2.1-toml-edit-0.1.4-1.drv'.
error: build of '/nix/store/hv7byrdjs254vc200cda7b51faj58bsa-integration-nightly-rocks.nvim-scm-1.drv', '/nix/store/z7fb1pa3gbd7h7n4n1bvdh0pd7r7zhzl-luajit2.1-toml-edit-0.1.4-1.drv' failed

@simifalaye
Copy link
Contributor

simifalaye commented Nov 11, 2024

@mrcjkb the issue is that when using key-tables in lua, the order of the key-value pairs isn't guaranteed in the same way as if you were using list tables with an integer index and is up to the optimization of the language. So, when it goes to compare the strings, the version and pin may show up in a different order causing the entire string to not be equal to the expected. There are a few ways this could be resolved, but I'm not sure which one would be the simplest/cleanest way for testing. You could even go as far as adding a toml library to the test dependencies then pass both toml strings into the library so that you can compare them for equality (there's toml for example). This is what I was considering doing before but I'm not sure if that's worth it just for testing purposes. You could maybe sort the lines of the resultant toml string to guarantee the order? Let me know if you think of something simpler/cleaner and I could implement it too if you're too busy now (as I know you have a lot on your plate)

EDIT: Yeah, I think the best bet is using the lua toml library for comparison of the two result strings. Just looked at the library and it should be quick to add into your PR. You can parse the expected and resultant strings into the table using the decode function, and finally compare the two tables using assert.same

@mrcjkb
Copy link
Member Author

mrcjkb commented Nov 11, 2024

@simifalaye thanks for the input 🙏
As it's not an actual bug and only affects the test suite, I wouldn't see this as the highest priority.
I'll look into fixing the test cases later 😄

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