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

Add some tests for configuration #57

Merged
merged 6 commits into from
Aug 10, 2022
Merged

Add some tests for configuration #57

merged 6 commits into from
Aug 10, 2022

Conversation

josecelano
Copy link
Member

@josecelano josecelano commented Aug 9, 2022

Hi @WarmBeer I have added some tests for the configuration.

New dependencies

  • uuid. I needed to create a unique id for a temporary config file like this test_config_231863df-a1d5-4dfc-a2a9-a8b054c0b8cf.toml.
  • serde_with. For the HttpTrackerConfig, you are using a custom serializer, but the deserialization is not working. The config options ssl_cert_path = "" and ssl_key_path = "" are being deserialized like Some("") instead of None.

Tests

  • For now I have only added a few tests for saving and loading the configuration to and from the config.toml file.

Changes

  • I had to add some new attributes to some struts (PartialEq, Debug) to compare two configurations.
  • I had to change the signature of two methods: save_to_file and load_from_file, to make those methods testable. I need to create the config file in a temporary file.

Questions

  1. I've added a tests for check default values only for one option (should_have_a_default_value_for_the_log_level). Do you think we should add one for each option or one test for all options, or maybe it does not makes sense to test default values directly?

  2. Why are there to impl Configuration blocks?

  3. I choose those test names in order to get this output when you execute them:

 cargo nextest run
   Compiling torrust-tracker v2.3.0 (/home/josecelano/Documents/github/committer/josecelano/torrust/torrust-tracker)
    Finished test [optimized + debuginfo] target(s) in 12.91s
  Executable unittests src/lib.rs (target/debug/deps/torrust_tracker-d1fbcd787d97cc63)
  Executable unittests src/main.rs (target/debug/deps/torrust_tracker-8d19522d4b2f8cf0)
    Starting 7 tests across 2 binaries
        PASS [   0.002s] torrust-tracker config::configuration::should_be_loaded_from_a_config_file
        PASS [   0.002s] torrust-tracker config::configuration::should_be_saved_in_a_toml_config_file
        PASS [   0.002s] torrust-tracker tracker::key::tests::auth_key_from_buffer
        PASS [   0.002s] torrust-tracker tracker::key::tests::auth_key_from_string
        PASS [   0.002s] torrust-tracker tracker::key::tests::generate_expired_auth_key
        PASS [   0.002s] torrust-tracker config::configuration::should_have_a_default_value_for_the_log_level
        PASS [   0.002s] torrust-tracker tracker::key::tests::generate_valid_auth_key
------------
     Summary [   0.004s] 7 tests run: 7 passed, 0 skipped

You read the lines like this: config::configuration::should_be_loaded_from_a_config_file

I will add some more tests.

@josecelano josecelano marked this pull request as draft August 9, 2022 16:47
src/config.rs Outdated Show resolved Hide resolved
@mickvandijke
Copy link
Member

Questions

  1. I've added a tests for check default values only for one option (should_have_a_default_value_for_the_log_level). Do you think we should add one for each option or one test for all options, or maybe it does not makes sense to test default values directly?
  2. Why are there two impl Configuration blocks?
  1. I don't think it would be useful to test these default values.
  2. I've never noticed this hehe, the functions can just be under one impl Configuration.

@josecelano
Copy link
Member Author

hi @WarmBeer It seems these two methods are not being used anymore:

    pub fn load(data: &[u8]) -> Result<Configuration, toml::de::Error> {
        toml::from_slice(data)
    }

    pub fn load_file(path: &str) -> Result<Configuration, ConfigurationError> {
        match std::fs::read(path) {
            Err(e) => Err(ConfigurationError::IOError(e)),
            Ok(data) => {
                match Self::load(data.as_slice()) {
                    Ok(cfg) => {
                        Ok(cfg)
                    }
                    Err(e) => Err(ConfigurationError::ParseError(e)),
                }
            }
        }
    }

@josecelano josecelano marked this pull request as ready for review August 10, 2022 09:01
@josecelano
Copy link
Member Author

I'm done with this PR.

@josecelano josecelano merged commit 065ca80 into development Aug 10, 2022
@josecelano josecelano deleted the config-tests branch August 10, 2022 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants