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

Update product metrics #2857

Merged
merged 14 commits into from Dec 24, 2021
Merged

Update product metrics #2857

merged 14 commits into from Dec 24, 2021

Conversation

josemigallas
Copy link
Contributor

@josemigallas josemigallas commented Dec 16, 2021

Description

This PR does 2 things:

  • Fixes n+1 query in MetricsIndexPresenter by using includes to load proxy_rules for each metric.
  • Update old 🥒 tests:
    • Combines related both metrics and methods tests in the file (they are all Metric)
    • Uses standard names (create vs. creation, delete vs. deletion)
    • Replaces nukes and zombies for Italian dishes
    • Updates scenarios so that they take into account the new 2 tabs
    • Removes a couple of redundant steps related to ParameterType provider
    • DRYs step provider should have metric by using ParameterType should

Jira

THREESCALE-7987: Add test for metrics index

@josemigallas josemigallas requested a review from a team December 16, 2021 14:10
@josemigallas josemigallas self-assigned this Dec 16, 2021
@akostadinov
Copy link
Contributor

Steps look good. It is hard to reason about the test changes though. It is not possible to see what changed from original version. Could you try to submit functional changes separate from refactoring?

@josemigallas
Copy link
Contributor Author

Steps look good. It is hard to reason about the test changes though. It is not possible to see what changed from original version. Could you try to submit functional changes separate from refactoring?

I don't think there are that many functional changes, it's mostly refactoring. I reordered the commits so that it's easier to understand the changes (hopefully).

I also added a changes summary in the description.

@akostadinov
Copy link
Contributor

akostadinov commented Dec 17, 2021

commit "updates cucumbers" is what renames files and can't be reviewed what is changed and what is new. Others look nice.

Co-authored-by: Ramihajamalala Hery <hramihaj@redhat.com>
features/step_definitions/metric_steps.rb Outdated Show resolved Hide resolved
features/step_definitions/metric_steps.rb Outdated Show resolved Hide resolved
features/api/metrics/create.feature Outdated Show resolved Hide resolved
features/api/metrics/index.feature Outdated Show resolved Hide resolved
features/api/metrics/update.feature Outdated Show resolved Hide resolved
features/support/parameter_types.rb Outdated Show resolved Hide resolved
transformer: ->(name) { Metric.find_by!(system_name: name) }
transformer: ->(name) {
metrics = Metric.where(parent_id: nil)
metric = metrics.find_by(friendly_name: name) || metrics.find_by(system_name: name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Searching across all metrics in the table by system_name is less prone to finding the wrong one, than by friendly_name, yet even so not risk free. This seems to increase the risk of fetching the wrong metric when there are other tests running in parallel on the same db. Do we really want it to be unscoped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just think it's far more readable to use the friendly name in the scenarios. I wouldn't call it risky but... 🤷🏻‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this one is fine. There should not be other tests running in parallel on the same table, there were parallel tests before and they are scoped on DB name level.

On new Rails version it is also the case https://guides.rubyonrails.org/testing.html#parallel-testing-with-processes

When parallelizing tests, Active Record automatically handles creating a database and loading the schema into the database for each process. The databases will be suffixed with the number corresponding to the worker. For example, if you have 2 workers the tests will create test-database-0 and test-database-1 respectively.

hallelujah
hallelujah previously approved these changes Dec 23, 2021
features/api/metrics/delete.feature Outdated Show resolved Hide resolved
Copy link
Contributor

@damianpm damianpm left a comment

Choose a reason for hiding this comment

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

LGTM

@josemigallas josemigallas merged commit 98faf03 into master Dec 24, 2021
@josemigallas josemigallas deleted the update_product_metrics branch December 24, 2021 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants