code-dot-org icon indicating copy to clipboard operation
code-dot-org copied to clipboard

move shared/middleware to new dashboard_legacy directory

Open davidsbailey opened this issue 3 years ago • 0 comments

Background

Finishes XTEAM-391. Depends on https://github.com/code-dot-org/code-dot-org/pull/47497 to be merged first. For other PRs which came before this, see: https://github.com/code-dot-org/code-dot-org/search?q=XTEAM-391&type=issues

Ideally, this code would go in dashboard, but this was not practical because:

  • the dashboard unit test suite uses a long-running transaction which stays open for the entire length of the test suite run
  • middleware tests depend on VCR, and our use of VCR requires us to reset the auto-increment counter on various tables before each VCR test case, which requires getting a table lock. this operation times out, which I confirmed is because the lock is held by the long-running transaction. For discussion on removing that transaction, see slack.

I also tried putting this code and tests in dashboard while partitioning the tests away from the dashboard unit test suite. While this seemed like it might work, I stopped because it seemed like it was adding more complexity and confusion by intermixing tests that were incompatible with each other.

Description

This PR does the following:

  • sets up a dashboard_legacy/ top-level directory whose job it is to hold code which belongs to dashboard, but cannot actually go in the dashboard directory because of technical constraints. For more details, see: https://github.com/code-dot-org/code-dot-org/blob/db019f16f45d46cb23ceeb776ef4c7528e98400e/dashboard_legacy/README.md
  • adds rake tasks to make dashboard_legacy tests run as part of our test suite
  • moves dashboard-specific projects code from shared/middleware to dashboard_legacy (this is what finishes XTEAM-391)

A side-goal of this approach is to set up the dashboard_legacy directory in a way that will let us move any other dashboard-specific code out of lib/ or shared/ and into a location that is earmarked for dashboard, without blocking on the resolution of the table lock issue described above. This means that future shared and lib code can be moved into dashboard_legacy, without having to do additional config work to set up new rake tasks as was done in this PR.

Testing story

  • [x] existing test coverage ensures I didn't break the existing shared or dashboard unit test suite, as well as that the dashboard application still loads and works properly
  • [ ] verified that dashboard_legacy tests are getting run in the drone output
  • [x] rake test:dashboard_legacy is running the moved tests locally, and they are passing
  • [ ] rake test:changed runs dashboard_legacy tests

Follow-up work

There were a couple of other issues I ran into while trying to get the middleware tests to run inside the dashboard suite. Since I've gone through the effort of uncovering them, I am going to make a best effort attempt to fix these issues so they don't prevent us from consolidating test suites later:

  • move capture_queries out of common_test_helper.rb and into pegasus, to avoid name collisions with similar methods defined in dashboard
  • remove mock Projects from dashboard test_helper.rb, which breaks middleware tests' assumption that it is using the real Projects defined in projects.rb

davidsbailey avatar Aug 09 '22 23:08 davidsbailey

Thank you for this feedback, @bethanyaconnor ! I am certainly open to other names. If we can't think of anything now, it should be easy to rename in the future, so long as we are on board with the overall approach and structure of what's in this directory.

to float one other idea, we could call this something like dashboard_split, which might give more emphasis to the idea that you should put code in here from shared or lib as you try to split dashboard from pegasus, and/or that this directory is tied to the separation effort. I do think good names are valuable, so I'm happy to bounce a few ideas back and forth on this.

davidsbailey avatar Aug 11 '22 17:08 davidsbailey