Refs #18921,#18922 - scenario tests and annotation #6716

jlsherrill · 2017-03-30T21:22:03Z

No description provided.

jlsherrill · 2017-03-30T21:22:58Z

Note this requires #6717 (Commit currently included here)

Lots of explanation here: https://github.com/Katello/katello/pull/6716/files#diff-0491b37365f9bbacc3357d8b9442422e

and a PR opening adding the first two workflows is here: theforeman/theforeman.org#845

A few questions arise from this PR, as this gives us the ability to do larger functional testing as part of our unit tests and maintain these annotations at the PR level:

Should we run these scenario tests as part of the normal unit tests against PRs (thats how the PR is now) Likely people would need to re-record more often
Should we add the 'rake katello:find_unannotated_requests' to the CI on unit tests? This would fail any PR that adds a request to the annotated workflows but does not update the annotations. We could instead add this to the nightly release job, so it doesn't fail the pr, but does fail the nightly.

ehelms · 2017-04-04T19:07:00Z

I'll start with some questions that come to mind:

How much time do these larger, functional tests add to our test suite?
If they are not run with the unit tests, when would we run them? And if they failed, what would we block on?
We could separate them out and add a parallel CI test
I tend towards catching things as soon as possible since the question becomes whose responsibility is it when things break. If we do it in the nightly release testing, then the bats nanny is in charge.
How often do you think these annotations should be needing updates?

ehelms · 2017-04-04T19:08:06Z

test/scenarios/annotations/README.md

+ export mode=all
+ ktest ../katello/test/scenarios/scenario_test.rb
+ unset mode
+```


Be great if we could capture this in a rake task or script or playbook.

yeah, i can do that

ehelms · 2017-04-04T19:11:03Z

test/support/annotations/templates/header.erb

+version: nightly
+foreman_version: nightly
+script: osmenu.js
+---


This looks like a Jekyll template, I'm just not clear on what it is doing here.

The purpose is to be published on theforeman.org, so this is adding the page header in there for that.

ehelms · 2017-04-04T19:13:58Z

test/scenarios/annotations/README.md

+  path: "/some/path"
+  title: 
+  description: 
+```


If the current tooling can generate this information for us, why do we need to store it in git?

It generates default matchers, but:
a) doesn't know the title and description
b) sometimes the default matching mechanism (path and method) aren't enough and you have to use something like 'starts_with' for more complex matching.

jlsherrill · 2017-04-04T19:17:37Z

1. How much time do these larger, functional tests add to our test suite?

they added tests take 4.7 seconds on my box for: Org create Product create repo create repo sync

1. If they are not run with the unit tests, when would we run them? And if they failed, what would we block on?

I think running as unit tests is preferred, its more that people may become annoyed for having to re-record them frequently. The only other place i think we'd run them is part of the release pipeline (but again i think via unit tests is preferred).

1. We could separate them out and add a parallel CI test 1. I tend towards catching things as soon as possible since the question becomes whose responsibility is it when things break. If we do it in the nightly release testing, then the bats nanny is in charge.

I agree

1. How often do you think these annotations should be needing updates?

The annotations should only need to be updated if: a) a new request is made during the scenarios b) a request is no longer made that was being made during the scenarios c) Some request that was being made no longer matches its old annotation VCR cassettes will likely need to be re-recorded more often, any time some parameter changes on any request during the recorded scenarios.

…

ehelms · 2017-04-04T19:38:39Z

test/scenarios/annotations/README.md

+
+## Matching requests
+
+The default matching request matching uses 'method' and 'path', but sometimes path is not enough.


This part is not entirely clear to me reading through, maybe an example would help with respect to where and how I add this to an anotation.

ehelms · 2017-04-04T19:39:54Z

test/scenarios/annotations/README.md

+  title: 
+  description: 
+```
+Simply copy and paste that into your annotation yaml file.  Edit, adding a description and title.  


Are description and title required? If yes, can you add something that as part of checking for unannotated requests also "lints" the annotations to ensure these are filled out for each?

Title is required, description is not (since some requests are quite obvious). I've added a check to check for blank titles

ehelms · 2017-04-04T19:40:38Z

test/scenarios/annotations/README.md

+Run the rake task to generate documentation:
+
+```
+rake katello:create_annotated_output


The idea is this output gets copied to theforeman.org I take it? How often would this be done? Should we fail somewhere in the process to ensure the docs stay up to date?

This is is up for debate, but i think it should be the release nanny's duties. I don't think this needs to be done nightly or even weekly, but ideally would be updated with each release

So there would be a roughly 3 month gap between annotations. Would bleeding edge or newly added ones be more useful to Pulp and Candlepin or do we expect them to really just look at stable releases?

I expect them to more look at stable releases when helping debug issues. I might could see generating new documentation when large features are added to run by them, but I don't know how often that would even be relevant.

ehelms · 2017-04-04T19:41:42Z

test/scenarios/annotations/README.md

@@ -0,0 +1,77 @@
+# Annotated Requests


Could we link this from the top level README or somewhere else that is typically seen by a developer? The content living here is fine since its relative but I want to ensure it's easy to find for developers.

ehelms · 2017-04-04T19:42:34Z

test/support/annotations/templates/header.erb

+title: Annotated Requests
+version: nightly
+foreman_version: nightly
+script: osmenu.js


I wasn't seeing where this script was being used

its used here: https://github.com/Katello/katello/pull/6716/files#diff-a44bfcf674225591e4f7c7bb2275f8f4R4

Specifically this osmenu.js script (not the erb template).

oh i see what you mean

ehelms · 2017-04-04T19:42:45Z

test/support/scenario_support.rb

+  end
+
+  def create_org(org)
+    #@org = org


ehelms · 2017-04-04T19:43:14Z

test/support/scenario_support.rb

+require "active_support/concern"
+
+class ScenarioSupport
+  attr_accessor(:user)


Never seen attr_accessor defined with parens.

ehelms · 2017-04-04T19:45:05Z

test/support/annotations/vcr_request.rb

@@ -0,0 +1,34 @@
+module Annotations
+  class VcrRequest
+    attr_accessor :method, :path, :request_body, :response_body, :response_code


Minor point, but looking at the code I think most if not all of these could just be attr_reader for outside access.

ehelms · 2017-04-04T19:45:46Z

test/support/annotations/vcr_request.rb

+  class VcrRequest
+    attr_accessor :method, :path, :request_body, :response_body, :response_code
+
+    def initialize(hash)


Consider using fetch on the hash to ensure each of the values below is defined.

ehelms · 2017-04-04T19:47:53Z

test/support/annotations/matched_annotation.rb

+        vcr_request.request_body.include?(request_body)
+      else
+        true
+      end


Consider reducing this to use a guard clause

ehelms · 2017-04-04T19:49:46Z

test/support/annotations/matched_annotation.rb

+          self.path = vcr_request.path
+        end
+        requests << vcr_request if matched
+      end


You could consider breaking this up into a series of selects pairing down the list in a sequence of steps. This could make any future debugging of this earlier as you can check the set of requests after each select to figure out where a particular request was removed from the selection.

i feel like using .selects() wouldn't work well for these conditions. Not to say this couldn't be cleaned up some

Perhaps, but as you can see thats me trying to read and interpret it. That each condition is skipping over an item in the list (i.e. reducing the list of items that ultimate action should be taken on).

ehelms · 2017-04-04T19:51:40Z

test/support/annotations/matched_annotation.rb

+      vcr_requests.each do |request|
+        found << request if self.matches?(request)
+      end
+      found


You could reduce this to a select

ehelms · 2017-04-04T19:52:16Z

test/support/annotations/matched_annotation.rb

+    def match_requests(vcr_requests)
+      found = []
+      vcr_requests.each do |request|
+        found << request if self.matches?(request)


I am not seeing where the method matches? is defined.

ah this is no longer used (was from a previous iteration)

ehelms · 2017-04-04T19:53:49Z

test/support/annotations/matched_annotation.rb

+module Annotations
+  class MatchedAnnotation
+    attr_accessor :annotation, :requests, :method, :path, :description, :title,
+                  :ignore_duplicates, :starts_with, :ignored_count, :request_body


Something to think about, if not all of these are being set outside this class they could be reduced to attr_reader and further reduced to just using @ if they aren't being read outside the class. This would reduce the "interface" surface of the class.

ehelms · 2017-04-04T19:54:36Z

test/support/annotations/annotation_writer.rb

+      File.read(File.dirname(__FILE__) + '/templates/annotation.erb')
+    end
+
+    def initialize(filename)


Maybe move this to the top since its the entry for the class

ehelms · 2017-04-04T19:55:02Z

test/support/annotations/annotation_writer.rb

+    end
+
+    def initialize(filename)
+      @file = File.open(filename, 'w')


Maybe add a safety check to ensure the file exists before opening?

and do what if it doesn't ? something other than raise an exception?

ehelms · 2017-04-04T19:56:52Z

test/support/annotations/annotation_writer.rb

+
+    def close
+      @file.close
+    end


Feels dangerous to have to explicitly remember to call to close the file. I am guessing you are doing this to be able to append to the file? And you want to do that in memory rather than writing a function that opens, writes in append mode and closes?

You have to 'remember' to do that when working with the normal file object. I would prefer this method rather than continually opening the file.

If you use a block with the File.open method you don't have to remember anything :)

ehelms · 2017-04-04T19:58:20Z

test/support/annotations/annotation_writer.rb

+      output = "# #{name}\n"
+      matched_annotations.each do |annotation|
+        output += ERB.new(annotation_template).result(binding)
+        count += 1


It looks like this count is arbitrarily being incremented. Where is it being used?

ehelms · 2017-04-04T19:59:46Z

test/support/annotations/annotation_file.rb

+    end
+
+    def self.load_annotations(directory)
+      Dir["#{directory}/*.yaml"].map do |file|


Could be useful to check if the directory exists and throw an error when trying to debug.

the directory is defined in git, i don't think its not going to exist unless for some reason you think the user will delete it by mistake?

I try not to predict user behavior. I am just thinking that if this produces an empty set (for some reason) you might chase the rabbit before realizing it was just the directory it expected didn't exist.

ehelms · 2017-04-04T20:01:17Z

test/support/annotations/annotation_file.rb

@@ -0,0 +1,35 @@
+module Annotations


Not that I expect us to add (but you never know what a plugin may do) a gem by this name, but there are some in existence and I could see one coming along and being used. Call it future proofing but you may consider wrapping this in module Katello.

ehelms · 2017-04-04T20:02:14Z

lib/katello/tasks/annotate_scenarios.rake

+  CASSETTE_DIR = "#{Katello::Engine.root}/test/fixtures/vcr_cassettes/scenarios/".freeze
+  OUTPUT_DIR = "#{Rails.root}/tmp/".freeze
+
+  desc ""


Were you intending to have a description? Makes it easier to find if you forget the name with rake -T

ehelms · 2017-04-04T20:37:46Z

Based on your responses, seems like this should only change if a user is editing tests and such where it would affect the annotations (and not say on a random UI PR fix) so I lean towards running this with normal unit tests and blocking a PR if the annotations are out of date.

jlsherrill · 2017-04-05T01:17:31Z

I think i've addressed or commented on all your comments

bbuckingham · 2017-04-27T14:09:36Z

test/scenarios/annotations/repository_create.yaml

+- method: get
+  path: "/candlepin/environments/119c4753ff6d3b7bd0b76de6d5a5f94a"
+  title: Retrieve candlepin environment
+  description: Retrieve the environment object (TODO WHY?)


Any update on the TODO?

bbuckingham · 2017-04-27T15:51:20Z

@jlsherrill, nice work! ACK to the approach/discussion and we can evolve the process, if needed, as we begin using it.

If possible, it may be good to summarize here or email info such as:

expectation/impact for developers
specify when they will be run (e.g. during UT)

Basically, give the devs a heads-up, since they may not all be following the PR. :)

This adds a handful of scenario tests, that is testing full stack integration by executing dynflow actions and recording their results in VCR. Only a few scenarios are tested so far, but more can be easily added. In addition this adds tooling to generat annotated documentation around these workflows and requests. See test/scenarios/annotations/README.md for more info.

theforeman-bot added Needs testing Not yet reviewed labels Mar 30, 2017

jlsherrill added the Blocked by another PR label Mar 30, 2017

jlsherrill force-pushed the scenarios branch 2 times, most recently from a9b8ab1 to ab9f441 Compare March 31, 2017 18:09

jlsherrill mentioned this pull request Mar 31, 2017

Adding katello request annotations theforeman/theforeman.org#845

Merged

ehelms reviewed Apr 4, 2017

View reviewed changes

test/support/scenario_support.rb Outdated

end

def create_org(org)

#@org = org

Copy link

Member

ehelms Apr 4, 2017

Choose a reason for hiding this comment

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

org is org

ehelms reviewed Apr 4, 2017

View reviewed changes

jlsherrill force-pushed the scenarios branch from ab9f441 to 968fd50 Compare April 5, 2017 01:14

bbuckingham self-assigned this Apr 24, 2017

bbuckingham reviewed Apr 27, 2017

View reviewed changes

bbuckingham approved these changes Apr 27, 2017

View reviewed changes

jlsherrill force-pushed the scenarios branch from 968fd50 to afe8093 Compare May 3, 2017 20:38

jlsherrill force-pushed the scenarios branch from afe8093 to f02d3c0 Compare May 4, 2017 15:07

jlsherrill merged commit fdc3b7b into Katello:master May 7, 2017

jlsherrill deleted the scenarios branch May 7, 2017 01:36


		## Matching requests

		The default matching request matching uses 'method' and 'path', but sometimes path is not enough.

Refs #18921,#18922 - scenario tests and annotation #6716

Refs #18921,#18922 - scenario tests and annotation #6716

Conversation

jlsherrill commented Mar 30, 2017

jlsherrill commented Mar 30, 2017 • edited

ehelms commented Apr 4, 2017

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

jlsherrill commented Apr 4, 2017 via email • edited

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

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

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

ehelms commented Apr 4, 2017

jlsherrill commented Apr 5, 2017

Choose a reason for hiding this comment

bbuckingham commented Apr 27, 2017

jlsherrill commented Mar 30, 2017 •

edited

jlsherrill commented Apr 4, 2017 via email •

edited