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 tests for the Config class #248

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

fauneau
Copy link

@fauneau fauneau commented Mar 29, 2024

The main goal of this pull request is to add tests to the Config class.

Additional changes:

  • Created class constant _CONFIG_FILE to replace string literal "config.toml".
    This is necessary for testing purposes and good practice in general.
  • Created class constant _SAMPLE_CONFIG_FILE to replace string literal "sample.config.toml".
    This is necessary for testing purposes and good practice in general.
  • Renamed save_config to dump_config
    To have the same function names as toml
  • Added ".vscode/" to .gitignore
    This is useful not to commit VS Code project settings

This pull request includes the commit from #221, so I will close it.

@fauneau
Copy link
Author

fauneau commented Mar 31, 2024

Following #24 major changes, there is some conflict on this branch.
I will resolve it and check everything works as it should on tuesday. (->Easter)

fix: eliminate the reloading of the config file at each call to Config.
fix and refactor: create the setup fixture and add it to each tests
fix: delete the instance before loading in
    test_save_then_load_keep_config_unchanged
This allows the tests not to depend on the implementation
@ARajgor
Copy link
Collaborator

ARajgor commented Apr 3, 2024

is it working?

@fauneau
Copy link
Author

fauneau commented Apr 3, 2024

Good catch! It wasn't working properly. The major changes that happened added some setters that I forgot to update when rebasing my work. The setters are never used (except for one of my tests), if vscode is to be trusted, so it would have been hard to see.

It's fixed now. For the rest, it works properly for me.

My changes are mostly renames and variable creations. The 4 created tests pass as well.

Screenshot 2024-04-04 010026

@ARajgor
Copy link
Collaborator

ARajgor commented Apr 6, 2024

so can you add one functionality? when the config.sample.toml file changes like added the new variables it automatically update the config.toml without touching the API keys values. just adding the new variables.

also why do we need a dedicated file for testing config?

@fauneau
Copy link
Author

fauneau commented Apr 6, 2024

I'm not sure what you mean. So, please tell me if I understand it correctly.
You would like Config to read sample.config.toml, then add all the variables from it to config.toml without their values (which are just filler values anyways).

I have some questions about this.
We can't assume config.toml exists. So, I'm guessing we're doing it only if it exists, correct?
And, is that really necessary? We can fill the values from the UI even if the variables don't appear in config.toml, right?

About the dedicated file for testing config, are you speaking of having a file that contains all the tests for the class Config? If so, this is the usual way to go about testing classes. Or, about the temporary file that those tests use? In this case, this is the easiest way to check if saving and loading config data works while keeping those tests implementation-agnostic. If we wanted to remove it, we could maybe simulate the temporary file or we can tie the tests to calls to toml functions.

Comment on lines 59 to 62




Choose a reason for hiding this comment

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

NIT:
Delete extra lines

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.

None yet

3 participants