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

Integration tests refactor #26

Merged
merged 4 commits into from
Apr 17, 2019
Merged

Integration tests refactor #26

merged 4 commits into from
Apr 17, 2019

Conversation

dansimau
Copy link
Contributor

@dansimau dansimau commented Feb 8, 2019

This refactors the integration tests to run in-process instead of compiling and executing the binary separately.

  • Move integration tests to tests/ directory

    Integration tests that execute Terraform are now in the top-level directory.

  • Use functional options pattern

    This is a breaking change that introduces the functional options pattern for astro.Project so we don't need to make breaking changes in the future.

  • Refactor cmd package

    The CLI application and state is now part of a struct that can be instantiated. Global variables have been removed. This improves testability as well as understanding of what's going on. Code has been cleaned up to be more readable.

Since this is a breaking API change we'll bump a minor version.

@dansimau dansimau self-assigned this Feb 8, 2019
@dansimau dansimau requested a review from DragonZ February 8, 2019 15:50
@dansimau
Copy link
Contributor Author

dansimau commented Feb 8, 2019

TODO: Need to update changelog.

astro/astro.go Show resolved Hide resolved
astro/cli/astro/cmd/main.go Outdated Show resolved Hide resolved
astro/cli/astro/main.go Show resolved Hide resolved
astro/cli/astro/cmd/cmds.go Outdated Show resolved Hide resolved
@dansimau dansimau force-pushed the integration-tests-refactor branch from 564fec7 to 44a91d7 Compare April 17, 2019 10:03
Copy link

@DragonZ DragonZ left a comment

Choose a reason for hiding this comment

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

I always feel confused by the struct/file/directory names in astro. For example,

Under astro/astro/, there is a file named astro. The top level Project struct is here.

type Project struct {
	config            *conf.Project
	sessions          *SessionRepo
	terraformVersions *tvm.VersionRepo
}

Inside the Project struct, there is a config member variable whose type is Project from conf package. But when I go inside conf package, I could not find some file named project.go and in the end I find another file named astro.go which defined conf.Project.

Similarly in astro/astro/config.go, instead of seeing some config struct and its methods, there are constructor functions for different Project structs.

func NewConfigFromFile(configFilePath string) (*conf.Project, error)
func NewProjectFromConfigFile(configFilePath string) (*Project, error)
func NewProjectFromYAML(yamlBytes []byte) (*Project, error)
func configFromYAML(yamlBytes []byte, rootPath string) (*conf.Project, error)

Without IDE's help, I could easily get lost navigating between different files during the review. Maybe we can give them more intuitive names.

astro/cli/astro/cmd/cmd.go Show resolved Hide resolved
astro/cli/astro/cmd/cmd.go Show resolved Hide resolved
@dansimau
Copy link
Contributor Author

I always feel confused by the struct/file/directory names in astro. For example,

These are really good comments. Let's file them away in an issue so we can fix this later. Maybe it could be a good task for you to take on? :-)

@dansimau dansimau merged commit a39cf79 into master Apr 17, 2019
@dansimau dansimau deleted the integration-tests-refactor branch April 17, 2019 14:39
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