Basic tests and parsing #1

YashoSharma · 2018-05-29T23:08:47Z

Description of changes:
Adds basic testing functionality and parsing of user inputs.

Adds test helper to assist in running integration tests
Adds basic integration test to ensure that generator functions and is compatible with aws-record
Adds unit tests for secondary index parsing
Adds secondary index parser
Adds unit tests for attribute parsing
Adds attribute parser

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

awood45

Biggest question is probably on the SecondaryIndex class - could we start with generating models generally, then add secondary index support as an iteration?

awood45 · 2018-05-29T23:23:23Z

COPYING

@@ -0,0 +1,202 @@
+


This looks like a verbatim copy of the LICENSE file included in the project template. For those files, let's use what Henri generated.

The guidelines for applying the Apache 2.0 License said "It is also recommended that you include a file called COPYING with the same content" as the License. Is it necessary then to have this file?

awood45 · 2018-05-29T23:24:18Z

aws-record-generator.gemspec

+
+  spec.summary       = "Rails generators for aws-record"
+  spec.description   = "Provides generators that integrate aws-record models with Rails scaffolding"
+  spec.homepage      = "https://github.com/awslabs"


https://github.com/awslabs/aws-record-generator rather than just https://github.com/awslabs

awood45 · 2018-05-29T23:25:09Z

aws-record-generator.gemspec

+  spec.require_paths = ["lib"]
+  spec.files = Dir['lib/**/*.rb']
+
+  spec.add_dependency('aws-record', '~> 1.0')


Let's depend on aws-record ~> 2

We want to be using the latest major version, and since any changes within that version are backwards compatible, we do not want to also lock on the minor version as this statement does.

awood45 · 2018-05-29T23:26:26Z

aws-record-generator.gemspec

+  spec.files = Dir['lib/**/*.rb']
+
+  spec.add_dependency('aws-record', '~> 1.0')
+  spec.add_dependency('rails', '~> 5.2.0')


This is far too narrow of a Rails dependency. Instead, we should understand the minimum version of Rails we need present to support generation (and indeed, possibly depend on a generator gem directly), and do a minimum dependency off that. See what we did in the aws-sdk-rails gemspec as an example.

awood45 · 2018-05-29T23:26:57Z

features/model/model.feature

@@ -0,0 +1,38 @@
+# Copyright 2018-2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.


Nitpick: It isn't 2019 yet, 2018-2018 is fine until 2019 comes.

This comment applies to all other files like this.

awood45 · 2018-05-29T23:30:56Z

features/model/model.feature

+    """
+    id:hkey count:int:rkey
+    """
+  When we run the ModelGenerator


Does this test imply we ran the ModelGenerator with zero options, and that the result is the table as specified?

We should circle back on this, I think what we would want to do for this test is probably this:

When we run the ModelGenerator with the following options: """ id:hkey count:int:rkey read_capacity:5 write_capacity:2 """ Then a "model" should be generated with contents: """ string_attr :id, hash_key: true integer_attr :count, range_key: true """ And a "table_config" should be generated with contents: """ t.read_capacity_units 5 t.write_capacity_units 2 """

...or similar. As written, the 5 and 2 look like magic numbers, and we should require those numbers be specified in the generator.

I'll work on updating the integration tests to address the comments, but did we want the number of r/w units to default to some arbitrary number if the user doesn't provide them, or should those parameters always be explicitly defined?

awood45 · 2018-05-29T23:34:27Z

features/model/model.feature

+    t.write_capacity_units 2
+    """
+
+  When we migrate the TableConfig


We should potentially make this a bit higher level. For example, what we are doing is running rails command line commands and expecting them to do certain things, this is true for the generator above. Perhaps they both actually look like this:

When we run the rails command line with the following command string: """ table_config:migrate """

The above could then also be:

When we run the rails command line with the following command string: """ generate aws_record:model id:hkey count:int:rkey read_capacity:5 write_capacity:2 """

Essentially, it's just running Rails command line tools like a user would (substituting the commands in our design doc as appropriate, these are loose illustrations).

awood45 · 2018-05-29T23:38:16Z

features/model/step_definitions.rb

+require 'generators/aws_record/model/model_generator'
+
+Before do
+  @gen_helper = AwsRecord::GeneratorTestHelper.new(AwsRecord::ModelGenerator, File.expand_path("tmp"))


I'm wondering if this is the appropriate scope for Cucumber tests from some of the other feedback. Is it possible, I'd wonder, to integration test actual rails command line commands? This is more relevant to the migration test (because as written it's redundant with aws-record integration tests), but something to look in to.

Addressed by 0989628

awood45 · 2018-05-29T23:39:58Z

spec/aws-record-generator/generators/aws_record/secondary_index_spec.rb

+require 'spec_helper'
+
+module AwsRecord
+  describe SecondaryIndex do


I feel like I'm missing something. Are we writing the Secondary Index generator before the generation of the standard hash and range key? It looks like we meant this to be something else.

I just wrote the simpler of the two parsers that the design document needed to orient myself with the new testing frameworks. I planned to write these two parsing modules before the core generator, but the SecondaryIndex will be integrated into the main generator at a later iteration.

awood45 · 2018-05-31T17:51:23Z

features/model/step_definitions.rb

+  @gen_helper.run_in_test_app cmd
+end
+
+Then("a {string} should be generated with contents:") do |generated_type, body|


Suggestion, but not a requirement: This is going to be a very fragile test. For example, whitespace changes will break it. There are three ways to deal with this:

Don't deal with it. This does work, so I'm okay with moving on with this for now.

Make this a file matching test. You'd create a fixtures folder with files which you could verify match the generated content. Something like Then("a {string} should be generated matching {string}") do |gtype, matching_filepath|

Write focused tests to do pattern matches on subsections of the file, where it would contain some set of lines, but we don't care about the rest.

Despite it not solving the fragility problem, I'd actually lean towards solution 2.

awood45 · 2018-05-31T18:08:04Z

spec/aws-record-generator/generators/aws_record/generated_attribute_spec.rb

+          end
+        end
+
+        it 'properly handles using options in combination with one another' do


The way we wrote this test raises an interesting question, in that we should validate that we accept the combinations provided.

It is NOT valid for an attribute to be both a hash key and a range key.

Keys cannot be any type, there are a subset of acceptable types.

We should actually delete this test, and replace it with three tests:

A set of cherry-picked, valid combinations of options.

An attribute being both a hash key and a range key, which should raise an exception.

An attribute being a hash key and also a map_attr type, which is invalid and should raise an exception (we can chat about how to write this logic going forward).

We should then ensure that the previous test's implied behavior that a hash key can be a string doesn't break.

awood45 · 2018-05-31T19:39:52Z

lib/generators/generated_attribute.rb

+        type, opts = "string", type if OPTS.any? { |opt| type.include? opt }
+
+        opts = opts.split(',') if opts
+        type, opts = *parse_type_and_options(type, opts)


I think after this, right before initialization, is a good time for a validation step. However, I do not think we should necessarily raise here. Ideally, since multiple attributes could have validation errors, there is a way to raise them all at once.

awood45 · 2018-05-31T20:24:57Z

lib/generators/generated_attribute.rb

+# and limitations under the License.
+
+module AwsRecord
+  class GeneratedAttribute


Guiding Question: How do we surface documentation about these options to users? How do we populate the "help" command?

We should make sure we are documenting options early and often.

rails will automatically display a USAGE file that you provide (I haven't started writing this, but a bulk of it will probably be cleaned and formatted snippets from the SEP)

awood45 · 2018-05-31T20:26:52Z

spec/spec_helper.rb

+# and limitations under the License.
+
+require 'aws-record'
+require 'generators/secondary_index'


We should be able to just do require 'aws-record-generator' here. There should be something gem-level that consumes everything else.

awood45

Good to merge, the remaining comments are stylistic but should be addressed in future changes.

awood45 · 2018-06-04T21:11:45Z

features/model/model.feature

-    t.write_capacity_units 4
-    """
-
+  Then a "model" should be generated


I'd do something here that makes the fixture chosen clear. I do see some step_definition magic, but could these steps themselves call out the fixture file?

awood45 · 2018-06-04T21:13:12Z

fixtures/cucumber/model/TestModel1.rb

@@ -0,0 +1,8 @@
+require 'aws-record'
+
+class TestModel1


I would give these fixtures meaningful names. Even "BasicGeneratedModel", and as your set of fixtures grows with the feature set, you will have an easier time keeping track.

awood45 · 2018-06-04T21:14:05Z

lib/generators/generated_attribute.rb

      def parse(field_definition)
        name, type, opts = field_definition.split(':')
        type = "string" if not type
        type, opts = "string", type if OPTS.any? { |opt| type.include? opt }

        opts = opts.split(',') if opts
-        type, opts = *parse_type_and_options(type, opts)
+        type, opts = *parse_type_and_options(name, type, opts)


I don't believe the * character is necessary here, Ruby will split the array without the splat if it is size two.

YashoSharma added 5 commits May 29, 2018 16:03

Created basic project structure

8a9ce8e

Added basic integration test

c70ea45

Added tests for parsing secondary indexes

3d07bd6

Initial implementation of secondary index parser

1adc871

Add test helper

8212c90

YashoSharma requested a review from awood45 May 29, 2018 23:08

awood45 suggested changes May 29, 2018

View reviewed changes

YashoSharma added 6 commits May 30, 2018 11:57

Formatting and versioning fixes

7c19a61

Added generated attribute unit tests

56723d5

Removed COPYING

7f7b446

Wrote initial generated attribute implementation

589592e

Integration test using command line invocation

0989628

Clear temporary directory properly

f552078

awood45 suggested changes May 31, 2018

View reviewed changes

YashoSharma added 4 commits May 31, 2018 14:41

Added module require def and test fixtures

034cedb

Added more robust attribute validation

93e1fd4

Reworked parse error flow

e549aac

refactor cucumber fixture, and test helper

7c86795

YashoSharma changed the title ~~(WIP) Basic tests and parsing~~ Basic tests and parsing Jun 4, 2018

awood45 approved these changes Jun 4, 2018

View reviewed changes

YashoSharma merged commit 68598e4 into master Jun 4, 2018

YashoSharma deleted the BasicTestsAndParsing branch June 4, 2018 21:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic tests and parsing #1

Basic tests and parsing #1

YashoSharma commented May 29, 2018 •

edited by singku

Loading

awood45 left a comment

awood45 May 29, 2018

YashoSharma May 30, 2018 •

edited

Loading

awood45 May 29, 2018

awood45 May 29, 2018

awood45 May 29, 2018

awood45 May 29, 2018

awood45 May 29, 2018

awood45 May 29, 2018

YashoSharma May 30, 2018

awood45 May 29, 2018

awood45 May 29, 2018

YashoSharma May 30, 2018

awood45 May 29, 2018

YashoSharma May 30, 2018

awood45 May 31, 2018

awood45 May 31, 2018

awood45 May 31, 2018

awood45 May 31, 2018

awood45 May 31, 2018

YashoSharma May 31, 2018

awood45 May 31, 2018

awood45 left a comment

awood45 Jun 4, 2018

awood45 Jun 4, 2018

awood45 Jun 4, 2018

		@@ -0,0 +1,38 @@
		# Copyright 2018-2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.

Basic tests and parsing #1

Basic tests and parsing #1

Conversation

YashoSharma commented May 29, 2018 • edited by singku Loading

awood45 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YashoSharma May 30, 2018 • 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

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

awood45 left a 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

YashoSharma commented May 29, 2018 •

edited by singku

Loading

YashoSharma May 30, 2018 •

edited

Loading