Skip to content

Add some tests for configuration#57

Merged
josecelano merged 6 commits intodevelopmentfrom
config-tests
Aug 10, 2022
Merged

Add some tests for configuration#57
josecelano merged 6 commits intodevelopmentfrom
config-tests

Conversation

@josecelano
Copy link
Copy Markdown
Member

@josecelano josecelano commented Aug 9, 2022 •
edited
Loading

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 requested a review from mickvandijke August 9, 2022 16:39
@josecelano josecelano marked this pull request as draft August 9, 2022 16:47
mickvandijke
mickvandijke reviewed Aug 9, 2022
View reviewed changes
@mickvandijke
Copy link
Copy Markdown
Member

mickvandijke commented Aug 9, 2022

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
Copy Markdown
Member Author

josecelano commented Aug 10, 2022

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
Copy Markdown
Member Author

josecelano commented Aug 10, 2022

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

Reviewers

@mickvandijke mickvandijke mickvandijke approved these changes

Assignees

No one assigned

Labels

None yet

Projects

Torrust Solution
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@josecelano @mickvandijke