FI-3182 Inferno execute unified short ids option #540

Shaumik-Ashraf · 2024-10-04T22:43:56Z

Summary

Create command line option that allows for unified short ids --short-ids (alias: -r)

Testing Guidance See Below

checkout this branch
bundle exec inferno services start
verify old behavior

bundle exec inferno execute --suite infra_test --groups 1 --tests 4.01 --inputs suite_input:a outer_group_input:b inner_group_input:c test_input:d external_test1_input:asdf external_inner_group_input:asdf external_outer_group_input:asdf

verify new behavior

bundle exec inferno execute --suite infra_test --short-ids 1 4.01 --inputs suite_input:a outer_group_input:b inner_group_input:c test_input:d external_test1_input:asdf external_inner_group_input:asdf external_outer_group_input:asdf

this should raise an exception to prevent running 1.01 twice

bundle exec inferno execute --suite dev_validator -r 1 1.01 -i "url:https://hapi.fhir.org/baseR4" patient_id:1234321

codecov · 2024-10-04T22:45:11Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.08%. Comparing base (5b83481) to head (805f4a9).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #540   +/-   ##
=======================================
  Coverage   84.08%   84.08%           
=======================================
  Files         261      261           
  Lines       11392    11392           
  Branches     1252     1252           
=======================================
  Hits         9579     9579           
  Misses       1803     1803           
  Partials       10       10

Flag	Coverage Δ
backend	`92.18% <ø> (ø)`
frontend	`79.35% <ø> (ø)`

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.

Jammjammjamm · 2024-10-10T19:12:01Z

lib/inferno/apps/cli/execute.rb

+        end
+      end
+
+      def shorts


Is there a reason this isn't called short_ids?

Its returning the actual test entities instead of ids.

In that case, this method name makes no sense.

Jammjammjamm · 2024-10-10T19:18:41Z

lib/inferno/apps/cli/execute.rb

@@ -201,11 +266,11 @@ def print_error_and_exit(err, code)
      end

      def runnable_type(runnable)
-        if Inferno::TestSuite.subclasses.include? runnable
+        if Inferno::TestSuite.subclasses.include?(runnable) || runnable.ancestors.include?(Inferno::TestSuite)


if runnable < Inferno::TestSuite

Jammjammjamm · 2024-10-10T19:21:12Z

lib/inferno/apps/cli/execute.rb

+              # Since Inferno can only have one test_run executing at a time per test_session,
+              # and each call to `inferno execute` represents one test_session, we block until
+              # each runnable until result is achieved
+              block_until_result_for(runnable)


How are tests running asynchronously? Why does the short id require this change, wasn't this exact behavior already available just using test ids and group ids rather short ids?

My bad I mixed up executing test_run with instantiating test run. Yes jobs are already running synchronously and this is now removed.

Jammjammjamm · 2024-10-14T16:39:13Z

lib/inferno/apps/cli/execute.rb

+        # sort so if a user specifies `inferno execute --tests 1.01 --short-ids 1.02` it will run in order 1.01, 1.02
+        # although this will disallow if a user wanted to intentionally run `inferno execute --tests 1.02 1.01` in
+        # that order
+        @selected_runnables ||= validate_unique_runnables(shorts + groups + tests).sort do |a, b|


I don't think we should sort, I think we should execute things in the order specified by the user.

Jammjammjamm · 2024-10-14T16:41:28Z

lib/inferno/apps/cli/execute.rb

@@ -165,6 +191,41 @@ def dispatch_job(test_run)
        end
      end

+      def validate_unique_runnables(runnables)


Why do we care to validate this? Why should this prevent someone from running tests?

There was glitchy behavior observed when a user tried to run the same test entity multiple times in one execute call. It may relate to the fact that Inferno core will create a result once all the children of a test entity have a result, but I haven't exactly proved it's due to that. It's easier to prevent this behavior than fix it.

I get that the Inferno REST API allows re-running the same entity over one session, and one execute call is reflecting one session, but the client also blocks spamming requests and possibly filters the results returned. When we tackle the other bug or give inferno execute more control over sessions we can re-enable this behavior.

Or I could undo it and leave the sharp tools with the user?

There was glitchy behavior observed when a user tried to run the same test entity multiple times in one execute call. It may relate to the fact that Inferno core will create a result once all the children of a test entity have a result, but I haven't exactly proved it's due to that. It's easier to prevent this behavior than fix it.

This change is similar to the async test execution change where it doesn't seem related to the purpose of the ticket. We should connect and discuss problem this is intended to prevent, because I'm willing to bet that the fix is simpler than this code.

Jammjammjamm · 2024-10-14T16:43:46Z

lib/inferno/apps/cli/execute.rb

+
+        @shorts ||= options[:short_ids]&.map do |short_id|
+          find_by_short_id(test_groups_repo, short_id)
+        rescue StandardError => maybe_not_found_error # rubocop:disable Naming/RescuedExceptionsVariableName


find_by_short_id should do what it says without requiring rescuing errors.

…erno-core into fi-3182-clean-diff

Shaumik-Ashraf · 2024-10-18T14:47:15Z

Updated testing guidance

Checkout this branch
bundle exec inferno services start
verify old behavior

bundle exec inferno execute --suite infra_test --groups 1 --tests 4.01 --inputs suite_input:a outer_group_input:b inner_group_input:c test_input:d

verify new behavior

bundle exec inferno execute --suite infra_test --short-ids 1 4.01 --inputs suite_input:a outer_group_input:b inner_group_input:c test_input:d

this should ~~raise an exception to prevent running 1.01 twice~~ now work, and show two results from 1.01 (one from group 1, and one from the test)

bundle exec inferno execute --suite dev_validator -r 1 1.01 -i "url:https://hapi.fhir.org/baseR4" patient_id:1234321

…fix test_run

Jammjammjamm · 2024-10-23T12:19:20Z

lib/inferno/apps/cli/execute.rb

@@ -165,23 +172,54 @@ def dispatch_job(test_run)
        end
      end

+      def runnable_is_included_in?(runnable, maybe_parent)


This is dead code now, right?

Jammjammjamm · 2024-10-23T12:19:28Z

lib/inferno/apps/cli/execute.rb

+            run_one(suite, test_run)
+
+            # TODO: make results more intuitively ordered than just reversing
+            results = test_runs_repo.results_for_test_run(test_run.id).reverse


Here and in line 55, replace reverse with:

.sort { |result, other| result.runnable.short_id <=> other.runnable.short_id }

I asked for this change here and in lines 49 and 55 (now 48 and 54). Making the change in line 62 is not the same behavior and is not correct.

It's because rubocop says that this hits the cyclomatic complexity limit

Also suites don't have short ids so I had to make those if statements. Suites appear last my implementation.

I would actually argue to raise or disable the cyclomatic complexity limit in rubocop, but decided I'll ask that in a future PR

But the behavior is wrong.

loop do results += results.sort end results

is not the same as

loop do results += results end results.sort

Suites should also definitely come first.

results += results.sort is the desired behavior right?

Shaumik-Ashraf · 2024-10-24T15:42:23Z

Okay behavior should be correct now

manually cherrypick diffs

f87bbb7

Shaumik-Ashraf self-assigned this Oct 4, 2024

Shaumik-Ashraf requested a review from Jammjammjamm October 4, 2024 22:45

Shaumik-Ashraf mentioned this pull request Oct 10, 2024

FI-3275 debug: inferno execute input validation #542

Merged

Shaumik-Ashraf added 2 commits October 10, 2024 16:10

Merge branch 'main' into fi-3182-clean-diff

7fae4cb

Merge branch 'main' into fi-3182-clean-diff

b91012c

Jammjammjamm reviewed Oct 15, 2024

View reviewed changes

Shaumik-Ashraf added 9 commits October 17, 2024 13:27

fix subclass condition

f49e44a

undo sorting runnables

94e6d92

improve find_by_short_id

a8d9f5e

Merge branch 'fi-3182-clean-diff' of github.com:inferno-framework/inf…

9929ff2

…erno-core into fi-3182-clean-diff

Merge remote-tracking branch 'origin/main' into fi-3182-clean-diff

c15c916

spec and cop

87ff463

remove custom blocking

19bc4aa

rename shorts to runnables_by_short_id

1c302a4

Merge branch 'main' into fi-3182-clean-diff

8d88b42

Shaumik-Ashraf added 2 commits October 18, 2024 12:30

take unique results instead of runnables; rename selected_runnables; …

df7df95

…fix test_run

spec

5e8f165

Shaumik-Ashraf requested a review from Jammjammjamm October 18, 2024 16:38

Jammjammjamm reviewed Oct 23, 2024

View reviewed changes

Shaumik-Ashraf added 4 commits October 23, 2024 17:28

replace reverse with sort and remove dead function

aaed03a

Merge remote-tracking branch 'origin/main' into fi-3182-clean-diff

bb6258e

spec

c857c0c

cop

4b478a8

Shaumik-Ashraf requested a review from Jammjammjamm October 24, 2024 15:23

Shaumik-Ashraf added 3 commits October 24, 2024 11:34

only sort if an entire suite is ran

16a21ff

fix

2b5465c

sort entities in selected run

e0e6f05

put suites first

805f4a9

Jammjammjamm approved these changes Oct 24, 2024

View reviewed changes

Shaumik-Ashraf merged commit 1b13c9b into main Oct 24, 2024
10 checks passed

Shaumik-Ashraf deleted the fi-3182-clean-diff branch October 24, 2024 17:57

This was referenced Oct 28, 2024

FI-2727 Initial Implementation of Inferno (g)(10) Test Kit into CI inferno-framework/inferno-reference-server#182

Merged

Release v0.4.44 #552

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FI-3182 Inferno execute unified short ids option #540

FI-3182 Inferno execute unified short ids option #540

Shaumik-Ashraf commented Oct 4, 2024 •

edited

Loading

codecov bot commented Oct 4, 2024 •

edited

Loading

Jammjammjamm Oct 10, 2024

Shaumik-Ashraf Oct 17, 2024

Jammjammjamm Oct 17, 2024

Jammjammjamm Oct 10, 2024

Jammjammjamm Oct 10, 2024

Shaumik-Ashraf Oct 17, 2024

Jammjammjamm Oct 14, 2024

Jammjammjamm Oct 14, 2024

Shaumik-Ashraf Oct 17, 2024

Shaumik-Ashraf Oct 17, 2024

Shaumik-Ashraf Oct 17, 2024

Jammjammjamm Oct 17, 2024

Jammjammjamm Oct 14, 2024

Shaumik-Ashraf commented Oct 18, 2024 •

edited

Loading

Jammjammjamm Oct 23, 2024

Jammjammjamm Oct 23, 2024

Jammjammjamm Oct 24, 2024

Shaumik-Ashraf Oct 24, 2024 •

edited

Loading

Shaumik-Ashraf Oct 24, 2024

Shaumik-Ashraf Oct 24, 2024

Jammjammjamm Oct 24, 2024

Shaumik-Ashraf Oct 24, 2024

Shaumik-Ashraf commented Oct 24, 2024

FI-3182 Inferno execute unified short ids option #540

FI-3182 Inferno execute unified short ids option #540

Conversation

Shaumik-Ashraf commented Oct 4, 2024 • edited Loading

Summary

Testing Guidance See Below

codecov bot commented Oct 4, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shaumik-Ashraf commented Oct 18, 2024 • edited Loading

Updated testing guidance

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shaumik-Ashraf Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shaumik-Ashraf commented Oct 24, 2024

Shaumik-Ashraf commented Oct 4, 2024 •

edited

Loading

codecov bot commented Oct 4, 2024 •

edited

Loading

Shaumik-Ashraf commented Oct 18, 2024 •

edited

Loading

Shaumik-Ashraf Oct 24, 2024 •

edited

Loading