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

feat(yaml): add yaml as valid file format #1

Open
wants to merge 8 commits into
base: main-enterprise
Choose a base branch
from

Conversation

dabcoder
Copy link
Owner

To keep discussing github#700

@dabcoder dabcoder marked this pull request as ready for review November 26, 2024 09:52
@anderssonjohan
Copy link

@dabcoder Nice! Can you also update the README and remove the comment about .yaml being ignored? 🙏

@dabcoder
Copy link
Owner Author

dabcoder commented Dec 19, 2024

Thanks for the feedback @anderssonjohan. I can certainly update the README.
Test wise, as far as I can see the getRepoConfigs method does not return anything that can be unit tested (unless I am mistaken), and I am also unsure how to test the childPluginsList method (test the content of the childPlugins array?). Let me know if you have any pointers in that area.
EDIT: getRepoConfigs does return an Object.

@anderssonjohan
Copy link

@dabcoder If I were you I would start by isolating the code under test into a helper function and test that. Then you only have to test the changed logic and not everything around it.

  1. Extract function, something like:
function getRepoOverrideConfig(repoConfigs, repository) {
  return repoConfigs[`${repository.repo}.yml`];
}
  1. Write failing test
const repoConfigs = { "repository.yaml": "yaml file was picked" };
const repo = { repo: "repository" };

expect(getRepoOverrideConfig(repoConfigs, repo).toEqual("yaml file was picked");
  1. Update the helper with your suggested changes
  2. Write a test for current behavior
const repoConfigs = { "repository.yml": "yml file was picked" };
const repo = { repo: "repository" };

expect(getRepoOverrideConfig(repoConfigs, repo).toEqual("yml file was picked");
  1. Document the precedence with a test
const repoConfigs = { "repository.yaml": "yaml file was picked", "repository.yml": "yml file was picked" };
const repo = { repo: "repository" };

expect(getRepoOverrideConfig(repoConfigs, repo).toEqual("yml file was picked");

@dabcoder
Copy link
Owner Author

● Settings Tests › getRepoOverrideConfig › repository defined in a file using the .yaml extension › Picks up a repository defined in file using the .yaml extension

    TypeError: Cannot read properties of undefined (reading 'repository.yml')

      415 |
      416 |   getRepoOverrideConfig(repoName) {
    > 417 |     return this.repoConfigs[`${repoName}.yml`] || this.repoConfigs[`${repoName}.yaml`] || {}
          |                            ^
      418 |   }
      419 |
      420 |   validate (section, baseConfig, overrideConfig) {

      at Settings.getRepoOverrideConfig (lib/settings.js:417:28)
      at Object.getRepoOverrideConfig (test/unit/lib/settings.test.js:156:37)

  ● Settings Tests › getRepoOverrideConfig › repository defined in a file using the .yml extension › Picks up a repository defined in file using the .yml extension

    TypeError: Cannot read properties of undefined (reading 'repository.yml')

      415 |
      416 |   getRepoOverrideConfig(repoName) {
    > 417 |     return this.repoConfigs[`${repoName}.yml`] || this.repoConfigs[`${repoName}.yaml`] || {}
          |                            ^
      418 |   }
      419 |
      420 |   validate (section, baseConfig, overrideConfig) {

      at Settings.getRepoOverrideConfig (lib/settings.js:417:28)
      at Object.getRepoOverrideConfig (test/unit/lib/settings.test.js:175:37)

@anderssonjohan
Copy link

@dabcoder You're passing the repoConfigs as the config object but they are not read from the config. They are loaded here:

this.repoConfigs = await this.getRepoConfigs(repo)

You can probably fix it by:

settings = createSettings({}).repoConfigs = ...

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