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 the extended configuration part in the 'default-config' output #112

Open
5 tasks
chshersh opened this issue Sep 20, 2022 · 4 comments
Open
5 tasks
Labels
config TOML configuration, config-related CLI options hacktoberfest https://hacktoberfest.com/ help wanted Extra attention is needed test
Milestone

Comments

@chshersh
Copy link
Owner

chshersh commented Sep 20, 2022

The default configuration provides an example of a more detailed config:

# [ripgrep] # Name of the tool (new or one of the hardcoded to override default settings)
# owner = "BurntSushi" # GitHub repository owner
# repo = "ripgrep" # GitHub repository name
# exe_name = "rg" # Executable name inside the asset
# Uncomment to download a specific version or tag.
# Without this tag latest will be used
# tag = "13.0.0"
# Asset name to download on linux OSes
# asset_name.linux = "x86_64-unknown-linux-musl"
# Uncomment if you want to install on macOS as well
# asset_name.macos = "apple-darwin"
# Uncomment if you want to install on Windows as well
# asset_name.windows = "x86_64-pc-windows-msvc"

However, this is not tested at all. The TOML format might diverge and there's no way to know if we need to change the default config. So I propose to automate testing by doing the following:

  • Implement to TOML serialization for ToolInfo
  • Write a property-based test that generated random ToolInfo and makes sure that decoding and encoding satisfy the roundtrip property
  • Generate the default config from a hardcoded ToolInfo without adding comments for the expanded version
  • Make sure that it parses and returns our hardcoded ToolInfo
  • Add comments for the generated config in the corresponding lines
@chshersh chshersh added help wanted Extra attention is needed test config TOML configuration, config-related CLI options labels Sep 20, 2022
@chshersh chshersh added this to the v0.3.0: Auto milestone Sep 20, 2022
@MitchellBerend
Copy link
Collaborator

Would the implementation of this serialization be implemented in the binary itself or only in the test?

Another question I have is do we add fuzzing for this test? This could potentially be flaky but I like the idea of testing random inputs.

@chshersh
Copy link
Owner Author

Would the implementation of this serialization be implemented in the binary itself or only in the test?

It'll be used only for tests. So I expect that it'll be stripped as an unused symbol (our current release process runs strip). But even if not, it's just a small part, so I'm not worried too much.

I propose to implement it in the binary. We might want to add a feature where we suggest putting the full table in the config, so this serialization might be handy.

Another question I have is do we add fuzzing for this test? This could potentially be flaky but I like the idea of testing random inputs.

Fuzzing would be nice. All fields in the ConfigAsset are optional. So, ideally, we want to test that they are parsed correctly. We have lots of fields so testing them manually leads to combinatorial explosion. That's why I proposed roundtrip property-based tests. But it can be done separately, no need to insert a random tool inside the default config. It'll complicate things without much benefit.

@MitchellBerend
Copy link
Collaborator

I have been thinking about this and what are we actually testing here? Initially I thought it was about the conversion to and from toml being commutative but reading more into this, that would mean the comments aren't of consequence.

Is the intent here just to check if the tests/default-config.toml is correct?

@chshersh
Copy link
Owner Author

@MitchellBerend Indeed, it's always good to revisit our assumptions 😌

In this issue, I want to only test the default-config.toml and make sure that the code under comments is valid. The way I imagined that is to implementing an encoding function for ToolInfo so it can be used in the generation of the default config. And since we have two functions (for decoding and encoding) it makes sense to test that they are consistent with each other.

But after checking the issue and conversation again, I've realised two things:

  1. The default config has comments for each field. We can't do this in the encoding function.
  2. The decoding function returns ConfigAsset while we're encoding ToolInfo. These are different types.

So yeah, we don't really need fuzzy testing. We just need to create a single const value that we're going to use in the default config and just format it in two ways: commented and uncommented (so we can test the decoding).

Maybe you can even use StaticToolInfo to define the hardcoded example easier 🤔

@chshersh chshersh added the hacktoberfest https://hacktoberfest.com/ label Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config TOML configuration, config-related CLI options hacktoberfest https://hacktoberfest.com/ help wanted Extra attention is needed test
Projects
None yet
Development

No branches or pull requests

2 participants