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

test installer #36

Merged
merged 6 commits into from
Sep 17, 2023
Merged

test installer #36

merged 6 commits into from
Sep 17, 2023

Conversation

Parker-Bartlett
Copy link
Contributor

@Parker-Bartlett Parker-Bartlett commented Sep 11, 2023

related to #31

A starting place for testing the installer. It also adds support for printing the installer version and showing the task's help doc when used without arguments. It mostly follows the testing strategy used in the Phoenix installer.

Screenshot 2023-09-16 at 7 57 28 PM Screenshot 2023-09-16 at 7 57 06 PM

@geolessel
Copy link
Owner

Oooooh, thank you so much! FWIW, I don't mind little PRs that chunk out the work. Even a PR that tests one little piece of functionality would be big enough to merge in IMO.

Let me know when you are comfortable with where this PR is at and we'll go from there.

- add flag support for installer version
- print mix help info when `mix vox.new` is ran without args
- add test for vox.new task
@Parker-Bartlett
Copy link
Contributor Author

@geolessel I'm all for keeping PRs small 😄

It should be ready to look over, let me know if there's anything I can update.

By the way, the last commit is adding the installer tests to CI, I can't validate that it works myself. I should be able to tell once the workflow runs on this PR.

@Parker-Bartlett Parker-Bartlett marked this pull request as ready for review September 16, 2023 23:50
Copy link
Owner

@geolessel geolessel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are fantastic! I have a style change request and one more thing that needs to be done.

Could you loosen the Elixir version restriction in the installer's mix.exs file to 1.14? That way it'll work with the testing workflow matrix versions.

end

defp random_string(len) do
len |> :crypto.strong_rand_bytes() |> Base.encode64() |> binary_part(0, len)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simple style pickiness I suppose but for piped logic I prefer one pipe per line. Mind changing it?

Suggested change
len |> :crypto.strong_rand_bytes() |> Base.encode64() |> binary_part(0, len)
len
|> :crypto.strong_rand_bytes()
|> Base.encode64()
|> binary_part(0, len)

@Parker-Bartlett Parker-Bartlett changed the title WIP - test installer test installer Sep 17, 2023
@geolessel
Copy link
Owner

geolessel commented Sep 17, 2023

@Parker-Bartlett Getting closer! It looks like we need to mix deps.get for the installer as well before we can test. Makes sense! 😆

CleanShot 2023-09-16 at 21 29 31@2x

@geolessel
Copy link
Owner

geolessel commented Sep 17, 2023

@Parker-Bartlett 🤔 and/or make ex_doc a :dev-only dep since it is the only dependency. I think I'd prefer the and version so if any dependencies are added later, they won't break the test runner.

defp deps do
  [
    {:ex_doc, "~> 0.30.6", only: :dev, runtime: false}
  ]
end

@Parker-Bartlett
Copy link
Contributor Author

Fingers crossed this is the one! And thanks for running CI on these 😆

Copy link
Owner

@geolessel geolessel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That did it! Thanks so much for these. I really appreciate it.

@geolessel geolessel merged commit 7781765 into geolessel:main Sep 17, 2023
4 checks passed
@Parker-Bartlett Parker-Bartlett deleted the test-installer branch September 18, 2023 13:37
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