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

Fi-1930: Custom Result Rollup #516

Merged
merged 46 commits into from
Sep 4, 2024
Merged

Conversation

vanessuniq
Copy link
Contributor

@vanessuniq vanessuniq commented Jul 15, 2024

Summary

This PR enhances the result rollup feature to support custom result logic through a user-defined block. The changes include:

  • New ResultCollection class: manages a collection of Inferno::Entities::Result objects
  • Updated TestGroup and TestSuite Classes: now has a customize_passing_result method that sets/ gets the custom block to define the passing criteria.
  • Updated Inferno::ResultSummarizer and Inferno::TestRunner: now takes custom result block into account when determining the overall result of a TestGroup or TestSuite.

Testing Guidance

  • Review changes.
  • Run unit tests and ensure passing
  • Run the new dev suite Custom Result Suite and ensure correct behavior

…pass

Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
@vanessuniq vanessuniq self-assigned this Jul 15, 2024
Copy link

codecov bot commented Jul 15, 2024

Codecov Report

Attention: Patch coverage is 96.61017% with 6 lines in your changes missing coverage. Please review.

Project coverage is 79.83%. Comparing base (9fab4c0) to head (f5e70ee).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...v_suites/dev_custom_results/custom_result_suite.rb 95.94% 3 Missing ⚠️
lib/inferno/dsl/messages.rb 85.71% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #516      +/-   ##
==========================================
+ Coverage   79.66%   79.83%   +0.17%     
==========================================
  Files         249      252       +3     
  Lines       13179    13305     +126     
  Branches     1283     1289       +6     
==========================================
+ Hits        10499    10622     +123     
- Misses       1931     1934       +3     
  Partials      749      749              
Flag Coverage Δ
backend 91.87% <96.61%> (+0.18%) ⬆️
frontend 74.54% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vanessuniq vanessuniq marked this pull request as draft July 19, 2024 15:05
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
@vanessuniq vanessuniq requested a review from arscan July 30, 2024 01:29
…pass

Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
…mework/inferno-core into fi-1930-custom-result-rollup
@vanessuniq vanessuniq marked this pull request as ready for review July 30, 2024 01:36
…pass

Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
…pass

Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
…mework/inferno-core into fi-1930-custom-result-rollup
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
title 'Passing Group'
description 'Criteria: Passes if test 1 passes, or test 2 and test 3 pass'

customize_passing_result do |results|
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is quite there yet.

  • customize_passing_result is a confusing name.
  • This doesn't allow groups to do all of the things with results that tests can do, like add messages and summary messages
  • It would be great if assertions and the rest of the test dsl could be used here

What if groups had the option of containing a run block like tests do, and the results were made available via an attr_accessor?

Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
@vanessuniq
Copy link
Contributor Author

Test suite and group result now have messages and summary message. Only summary message for group is shown on the UI. messages are no shown.
Screenshot 2024-08-09 at 9 42 07 AM

def_delegators 'self.class', :title, :id, :groups, :inputs, :outputs, :tests
def_delegators 'self.class', :title, :id, :block, :groups, :inputs, :outputs, :tests

attr_accessor :result_message, :child_results
Copy link
Collaborator

Choose a reason for hiding this comment

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

You think it's worth calling this child_results rather than just results?

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 wanted to add clarity that these are the children's results being used, but I'm totally fine simplifying it to results since it only handles those anyway.

end

describe '#each' do
it 'iterates over each result in the collection' do
Copy link
Collaborator

Choose a reason for hiding this comment

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

it 'returns.... You should check the return type of #each as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to test that it returns the correct thing when it's given a block, also.

expect(result_collection.each).to be_a(Enumerator)
end

it 'returns the `results` attribute of the collection when a block is given' do
Copy link
Collaborator

Choose a reason for hiding this comment

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

#each typically returns self, and I think it should here also, so that any further chaining happens on a ResultCollection rather than an Array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true!! updated

Signed-off-by: Vanessa Fotso <vfotso@mitre.org>

parent_instance = parent.new
parent_instance.results << child_results
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this actually tested? This should create a nested array, in contrast to the flat array created above.

Copy link
Contributor Author

@vanessuniq vanessuniq Sep 4, 2024

Choose a reason for hiding this comment

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

ResultCollection#<< flattens the ResultCollection#results array.

@vanessuniq vanessuniq merged commit 586d3b8 into main Sep 4, 2024
10 checks passed
@rpassas rpassas mentioned this pull request Sep 11, 2024
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