Skip to content

Clone feature states, feature value and feature segments on env clone - #242

Merged
gagantrivedi merged 18 commits into
mainfrom
fix/github-172/clone-state
Aug 10, 2021
Merged

Clone feature states, feature value and feature segments on env clone#242
gagantrivedi merged 18 commits into
mainfrom
fix/github-172/clone-state

Conversation

@gagantrivedi

Copy link
Copy Markdown
Member

closes #172

@gagantrivedi gagantrivedi changed the title wip: Make objects cloneable Clone feature states, feature value and feature segments on env clone Aug 5, 2021
@gagantrivedi
gagantrivedi marked this pull request as ready for review August 5, 2021 02:48

@matthewelwell matthewelwell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like we're missing quite a few tests now for each of the new clone methods. I thikn it might also be wise to put in a new full integration test similar to the ones that exist at the moment in tests/integration/.

Comment thread api/environments/models.py Outdated
Comment thread api/environments/tests/test_models.py Outdated
Comment thread api/features/feature_segments/tests/test_models.py
@gagantrivedi

Copy link
Copy Markdown
Member Author

Looks like we're missing quite a few tests now for each of the new clone methods. I thikn it might also be wise to put in a new full integration test similar to the ones that exist at the moment in tests/integration/.

I was planning to add tests to the test_models.py of the modules involved. if we bring that part to test/integration we will have model tests at two places(unless we say that the test_models is deprecated) what do you think?

@matthewelwell

Copy link
Copy Markdown
Contributor

The integration tests aren't really testing the models, they should be testing the API interface only. So, for example, all the fixtures are created by hitting the relevant API endpoints, not by using the Django ORM.

There is definitely some duplication / overlap but I quite like testing the more complicated flows with integration tests so that we can verify that the actual client experience is correct.

@matthewelwell matthewelwell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A few questions / comments.

Comment thread api/features/models.py Outdated
# Firstly, create an environment and update it's feature state

# Fetch the feature state id to update
feature_state = FeatureState.objects.get(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we do this without using the ORM?

)


def test_clone_environment_creates_permission_with_the_current_user(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we just bundle this into the test above and use the /environments/{api_key}/my-permissions/ endpoint to check the permissions?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think we should keep this in it's own function, since the above test is only responsible for making sure that we clone the feature states correctly(as the name suggests). wdyt?

)


def test_clone_environment_returns_200(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need this test at all? We're already checking the response is 200 in the next test, right?

assert json_response["results"][0]["id"] != feature_segment


def test_clone_clones_segments_overrides(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
def test_clone_clones_segments_overrides(
def test_env_clone_clones_segments_overrides(

)

# Then fetch the feature state of clone environment for the feature segement
clone_feature_segment_id = FeatureSegment.objects.get(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Again, can we do this without using the ORM?

@matthewelwell matthewelwell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approved with a couple of very minor points.

Comment thread api/tests/integration/helpers.py Outdated
Comment thread api/tests/integration/environments/test_clone_environment.py Outdated
@gagantrivedi
gagantrivedi merged commit 836f89d into main Aug 10, 2021
@gagantrivedi
gagantrivedi deleted the fix/github-172/clone-state branch August 10, 2021 03:12
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.

Clone environment

2 participants