notmarkmiranda commented 4 years ago

164

The UpdateModelController was overloaded with logic that made it difficult to test. The only way to properly test the controller was correctly parsing incoming parameters was with a request spec.

This change will allow unit and function tests to be written against the actions that are created. Errors bubble up from the base level, through the ModelUpdateAction and into the controller.

coleshaw commented 4 years ago

Looks like the name attribute comes back differently ... missing description and case difference for display_name. Does the json_attributes method do something to that attribute when it serializes?

notmarkmiranda commented 4 years ago

I'm not sure about the case difference for display_name or name coming back differently. I'm seeing the correct return on those options. Do you have a specific thing that you're checking that you can send me so I can recreate?

description is returned as desc from https://github.com/mountetna/magma/blob/master/lib/magma/attribute.rb#L61

coleshaw commented 4 years ago

I'm looking at the test failures and the logs, specifically lines 124 and 125 of the "run test suite" block:

https://github.com/mountetna/magma/pull/173/checks?check_run_id=767032672#step:8:124

It shows one failure:

   1) Magma Commands Magma::LoadProject loads attributes into the database
     Failure/Error: expect(json_attributes).to eq(model_json_template)

       -"name" => {"attribute_class"=>"Magma::Attribute", "attribute_name"=>"name", "attribute_type"=>"identifier", "desc"=>123, "display_name"=>"NAME", "hidden"=>false, "name"=>"name", "read_only"=>false, "restricted"=>false, "type"=>"String", "validation"=>nil},
       +"name" => {"attribute_class"=>"Magma::Attribute", "attribute_name"=>"name", "attribute_type"=>"identifier", "display_name"=>"Name", "hidden"=>false, "name"=>"name", "read_only"=>false, "restricted"=>false, "type"=>"String", "validation"=>nil},

It looks like the returned value is missing desc and the display_name value is Title case instead of ALL CAPS. Not sure what the source for each one is, though...

notmarkmiranda commented 4 years ago

Ah, that. I saw that on Friday at the end of my day and decided to leave it for Monday Mark to figure out.

I think it has to do with the ordering of the specs, running that single test (or even that whole file) on its own, and the test will pass, but in the whole suite, it fails. My assumption is that this is a failure to update and the comparison of the attributes is to the old one.

graft commented 4 years ago

In my hands I see three failures:

    rspec ./spec/server/update_model_spec.rb:27 # UpdateModelController updates attribute options
    rspec ./spec/server/update_model_spec.rb:70 # UpdateModelController does not update does not update attribute options with the incorrect data type
    rspec ./spec/magma_commands_spec.rb:9 # Magma Commands Magma::LoadProject loads attributes into the database

The first two are pretty alarming and involve insert_conflict failing:

+PG::UniqueViolation: ERROR:  duplicate key value violates unique constraint "attributes_project_name_model_name_attribute_name_index"
+DETAIL:  Key (project_name, model_name, attribute_name)=(labors, monster, name) already exists.

do you all see any of these?

notmarkmiranda commented 4 years ago

I fixed the first issue that @coleshaw pointed out, it was definitely an ordering issue. The magma_command_spec loads from the JSON file and compares against a call to Labors::Monster.json_template after the options for description and display_name had been updated.

To fix this, I set the update_model_spec to send another post to update the options back within the file.

The insert_conflict seems to be a similar issue in that if you run magma_project_spec before the update_model_spec, it will pass: bundle exec rspec spec/magma_project_spec.rb spec/server/update_model_spec.rb --order defined

If you run them in the opposite order, it fails every time: be rspec spec/server/update_model_spec.rb spec/magma_project_spec.rb --order defined

That seems to say that the insert conflict does work, but since the magma_project_spec doesn't implement insert_conflict, the failure is due to a duplicate attribute already existing.

coleshaw commented 4 years ago

Nice, good detective work!

graft commented 4 years ago

Although it passes here the test ./spec/magma_project_spec.rb:58 is now failing for me (in place of that other load_commands one, which now seems to be passing?).

This test is pretty interesting - when it was written, a scant few weeks ago, the norm was we would load the spec/labors/models for our model definitions, and nothing was written into the attributes table at load time. Now, that is no longer the case, because we load attributes from YAML fixtures in spec/spec_helper.rb.

However, in production we still have ipi models in the old way, that is, with .rb models that we haven't yet ported into attributes. This test would ensure that we could write new rows in attributes over those .rb-defined attributes and have them take precedence; but we no longer have any .rb attributes in the test suite!

notmarkmiranda commented 4 years ago

That's interesting. I've run the test suite several times and get no failures. Did you pull down the most recent changes?

Are you saying the test at ./spec/magma_project_spec.rb:58 (or in the file as a whole) is obsolete? The issue, I think is that when we wrote that test, there were no other tests dealing with inserting into the database. And now most of the tests we're working on now (and inevitably on the next set of tickets) will all be inserting into the database. I don't know a lot about database_cleaner, but is there a way to clear the db more frequently? Would that solve the problem?

coleshaw commented 4 years ago

Sorry, really silly question, but does there need to be one (or more) migrations in spec/labors/migrations to add the models table? When I try to run the tests locally, I get:

Sequel::DatabaseError:
  PG::UndefinedTable: ERROR:  relation "models" does not exist
# ./spec/spec_helper.rb:27:in `<top (required)>'
# ------------------
# --- Caused by: ---
# PG::UndefinedTable:
#   ERROR:  relation "models" does not exist
#   ./spec/spec_helper.rb:27:in `<top (required)>'
No examples found.

If I run MAGMA_ENV=test ./bin/magma migrate, I don't get the models table added, it just tries adding all the other labors test tables, like project, monsters, etc.

graft commented 4 years ago

There is a bin/magma global_migrate command now that builds the meta-tables.

coleshaw commented 4 years ago

Thanks, that helped. But I am definitely missing something key, because I don't have any of the models files (spec/labors/models/) required in spec/labors/requirements.rb:

$ bundle exec rspec spec/

An error occurred while loading spec_helper. - Did you mean?
                    rspec ./spec/spec_helper.rb

Failure/Error: require_relative 'models/codex'

LoadError:
  cannot load such file -- /home/developer/magma/spec/labors/models/codex
# ./spec/labors/requirements.rb:1:in `require_relative'
# ./spec/labors/requirements.rb:1:in `<top (required)>'

Should I be generating those model files, or grabbing them from elsewhere? They don't seem to be included in this PR.

alimi commented 4 years ago

Some of the failures that came up in this PR are caused by Magma::Attribute#update_option. It gets called as part of Magma::UpdateAttributeAction.

After updating the attribute's options in the database

    def update_option(opt, new_value)
      opt = opt.to_sym
      return unless EDITABLE_OPTIONS.include?(opt)

      database_value = new_value.is_a?(Hash) ? Sequel.pg_json_wrap(new_value) : new_value

      Magma.instance.db[:attributes].
        insert_conflict(
          target: [:project_name, :model_name, :attribute_name],
          update: { "#{opt}": database_value, updated_at: Time.now }
        ).insert(
          project_name: @model.project_name.to_s,
          model_name: @model.model_name.to_s,
          attribute_name: name.to_s,
          type: self.class.attribute_type,
          created_at: Time.now,
          updated_at: Time.now,
          "#{opt}": database_value
        )

it updates the options in memory


      instance_variable_set("@#{opt}", new_value)
      @validation_object = nil if opt == :validation
    end

Between tests, DatabaseCleaner will rollback changes in the database so the attribute changes shouldn't be hanging around when other tests are run. However, nothing will reset the instance variables that were changed on the attributes. So once tests are run that change attributes via Magma::Attribute#update_option, those changes hang around in memory causing other tests to fail.

Changes like https://github.com/mountetna/magma/pull/173/commits/890ae9e0ad6ac958a8aeaee20e671bc209d2c3c2 help fend off this problem, but we shouldn't have this problem in the first place. This problem also makes it difficult to wrap the actions in a transaction because changes are still in memory even if they're not committed in the database.

There are a couple of ways around this problem. One option is to make Magma::Attribute < Sequel::Model. Sequel::Model should give us better management of the attributes in memory. I made a little progress on Friday, and I'm gonna spend a little bit of time seeing if I can get it to work this morning. Otherwise, we'll have to change update_option so changes aren't so sticky in memory.

alimi commented 4 years ago

@coleshaw the Labors project models are now completely loaded from the database (https://github.com/mountetna/magma/pull/162). You can delete spec/labors/requirements.rb and the test suite should run.

coleshaw commented 4 years ago

Ah, thanks @alimi ! That works -- I'm confused at why the tests run in GH Actions, then ... should that file be removed from the PR?

For me locally, I get 4 test failures. It's kind of fascinating why we're all seeing different test results:

Failures:

  1) RetrieveController files retrieves file attributes with storage links
     Failure/Error: monster = create(:monster, :lion, stats: 'stats.txt')

     Sequel::DatabaseError:
       PG::InvalidTextRepresentation: ERROR:  invalid input syntax for type json
       LINE 1: ...at", "updated_at") VALUES ('Nemean Lion', 'lion', 'stats.txt...
                                                                    ^
       DETAIL:  Token "stats" is invalid.
       CONTEXT:  JSON data, line 1: stats...
     # ./spec/retrieve_spec.rb:101:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:182:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:181:in `block (2 levels) in <top (required)>'
     # ------------------
     # --- Caused by: ---
     # PG::InvalidTextRepresentation:
     #   ERROR:  invalid input syntax for type json
     #   LINE 1: ...at", "updated_at") VALUES ('Nemean Lion', 'lion', 'stats.txt...
     #                                                                ^
     #   DETAIL:  Token "stats" is invalid.
     #   CONTEXT:  JSON data, line 1: stats...
     #   ./spec/retrieve_spec.rb:101:in `block (3 levels) in <top (required)>'

  2) QueryController Magma::FilePredicate returns a url
     Failure/Error: lion = create(:monster, name: 'Nemean Lion', stats: 'lion-stats.tsv')

     Sequel::DatabaseError:
       PG::InvalidTextRepresentation: ERROR:  invalid input syntax for type json
       LINE 1: ...created_at", "updated_at") VALUES ('Nemean Lion', 'lion-stat...
                                                                    ^
       DETAIL:  Token "lion" is invalid.
       CONTEXT:  JSON data, line 1: lion...
     # ./spec/query_spec.rb:281:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:182:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:181:in `block (2 levels) in <top (required)>'
     # ------------------
     # --- Caused by: ---
     # PG::InvalidTextRepresentation:
     #   ERROR:  invalid input syntax for type json
     #   LINE 1: ...created_at", "updated_at") VALUES ('Nemean Lion', 'lion-stat...
     #                                                                ^
     #   DETAIL:  Token "lion" is invalid.
     #   CONTEXT:  JSON data, line 1: lion...
     #   ./spec/query_spec.rb:281:in `block (3 levels) in <top (required)>'

  3) QueryController Magma::FilePredicate returns a path
     Failure/Error: lion = create(:monster, name: 'Nemean Lion', stats: 'lion-stats.tsv')

     Sequel::DatabaseError:
       PG::InvalidTextRepresentation: ERROR:  invalid input syntax for type json
       LINE 1: ...created_at", "updated_at") VALUES ('Nemean Lion', 'lion-stat...
                                                                    ^
       DETAIL:  Token "lion" is invalid.
       CONTEXT:  JSON data, line 1: lion...
     # ./spec/query_spec.rb:281:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:182:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:181:in `block (2 levels) in <top (required)>'
     # ------------------
     # --- Caused by: ---
     # PG::InvalidTextRepresentation:
     #   ERROR:  invalid input syntax for type json
     #   LINE 1: ...created_at", "updated_at") VALUES ('Nemean Lion', 'lion-stat...
     #                                                                ^
     #   DETAIL:  Token "lion" is invalid.
     #   CONTEXT:  JSON data, line 1: lion...
     #   ./spec/query_spec.rb:281:in `block (3 levels) in <top (required)>'

  4) Magma::Migration update migrations makes no changes when removing child, collection or table attributes
     Failure/Error: expect(Labors::Monster.migration).to be_empty
       expected `#<Magma::UpdateMigration:0x000056494304b1a8 @model=Labors::Monster, @changes={"alter_table(Sequel[:labors][:monsters])"=>["set_column_type :stats, String"]}>.empty?` to return true, got false
     # ./spec/migration_spec.rb:199:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:182:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:181:in `block (2 levels) in <top (required)>'

Finished in 2.19 seconds (files took 1.59 seconds to load)
191 examples, 4 failures, 1 pending

Failed examples:

rspec ./spec/retrieve_spec.rb:99 # RetrieveController files retrieves file attributes with storage links
rspec ./spec/query_spec.rb:297 # QueryController Magma::FilePredicate returns a url
rspec ./spec/query_spec.rb:286 # QueryController Magma::FilePredicate returns a path
rspec ./spec/migration_spec.rb:190 # Magma::Migration update migrations makes no changes when removing child, collection or table attributes

alimi commented 4 years ago

spec/labors/requirements.rb was never committed. I never had it or needed it when I ran the test suite directly on machine, but I couldn't get the test suite to run on the VM without it. It never existed in the GitHub Actions either 🤷🏾‍♂️

coleshaw commented 4 years ago

Oh interesting...I didn't notice that. I must have added it manually too and forgotten about it!

alimi commented 4 years ago

tl;dr:

The original error @coleshaw called out happens on master
174 fixes these errors
We don't need commits https://github.com/mountetna/magma/pull/173/commits/890ae9e0ad6ac958a8aeaee20e671bc209d2c3c2 and https://github.com/mountetna/magma/pull/173/commits/c5c2306685de4cd737f073663e3aa0e3aaa45d42 in this PR
We haven't been able to reproduce @graft's errors 🤷🏾‍♂️

The original error @coleshaw called out (https://github.com/mountetna/magma/pull/173/checks?check_run_id=767032672#step:8:124) happens on master. Everyone should be able to recreate it locally by running

rspec spec/server/update_model_spec.rb spec/magma_commands_spec.rb --seed 18642

Nothing in this PR is causing this error. It happens because Magma::Attribute#update_option changes options in memory as described in my earlier comment https://github.com/mountetna/magma/pull/173#issuecomment-644144794. Fixing #update_option is a bigger challenge, but we can make the tests pass in the meantime. I opened https://github.com/mountetna/magma/pull/174 as one option.

https://github.com/mountetna/magma/pull/174 also updates spec/magma_project_spec.rb:58 so it makes more sense.

We started seeing more errors in this PR after https://github.com/mountetna/magma/pull/173/commits/890ae9e0ad6ac958a8aeaee20e671bc209d2c3c2 because it inserts data after DatabaseCleaner is called. DatabaseCleaner truncates the database only once when the suite is loaded.

  config.before(:suite) do
    # …
    DatabaseCleaner.clean_with(:truncation)
  end

Then using the transaction strategy, DatabaseCleaner will rollback changes around each test.

  config.around(:each) do |example|
    DatabaseCleaner.cleaning do
      example.run
    end
  end

https://github.com/mountetna/magma/blob/2a5deb4f0c7a97d07338b5d3128404340dcf28e1/spec/spec_helper.rb#L173-L185

https://github.com/mountetna/magma/pull/173/commits/890ae9e0ad6ac958a8aeaee20e671bc209d2c3c2 inserts data in an after(:all) hook after DatabaseCleaner has been run. As a result, test data persists in the database and now then we have to do stuff like https://github.com/mountetna/magma/pull/173/commits/c5c2306685de4cd737f073663e3aa0e3aaa45d42.

So if we merge in #174 and revert commits https://github.com/mountetna/magma/pull/173/commits/890ae9e0ad6ac958a8aeaee20e671bc209d2c3c2 and https://github.com/mountetna/magma/pull/173/commits/c5c2306685de4cd737f073663e3aa0e3aaa45d42, master should be green. There are other problems hanging around, but we can tackle them later.

graft commented 4 years ago

:ship:

mountetna / magma

Extract logic from update_model_controller #173

164

174 fixes these errors