[ENHANCEMENT] [MER-5670] Playwright test for Adaptive Lesson: BB Unit 7: Our Blue Planet - #6762
Conversation
AI Review —No issues found |
70fd3f1 to
7c11988
Compare
- OurBluePlanetTask.ts: Made lesson interactions more reliable by handling screens without feedback, waiting for stable inputs, and avoiding intentional wrong answers on completion-scored screens. - our-blue-planet-adaptive.spec.ts: Increased teardown timeouts and removed the previous 15-second timeout cap. - automation_setup.ex: Deletes every section belonging to the disposable project before deleting its resources, preventing foreign-key failures from sections included in the imported archive. - sections.ex: Clears a section’s root-resource reference before deleting the section, all within one transaction. - automation_setup_controller.ex: Extends Cowboy’s idle timeout specifically for teardown requests so large-course cleanup can finish. - sections_test.exs: Verifies deleting a section also removes its section resources. - automation_setup_controller_test.exs: Verifies project teardown also removes sections imported with the course archive.
AI Review — performanceNo issues found |
AI Review — securityNo issues found |
AI Review — elixirNo issues found |
simonchoxx
left a comment
simonchoxx
left a comment
There was a problem hiding this comment.
I was able to run the spec and it passed for me, and teardown seems to be working fine — I checked the DB afterwards and it came out clean.
One thing I noticed: if the server has been running for a while, the test starts failing, and not always in the same place. If I restart Phoenix right before, it passes. My guess is that when the server responds slower, the test retries clicks that had actually gone through and ends up submitting twice — sometimes it skips a screen, sometimes it re-submits the same one, and the score ends up below 100 without anything being raised.
Leaving it here mainly so it's on your radar.
| # resources | ||
| tests/resources/login.env No newline at end of file | ||
| tests/resources/login.env | ||
| local-assets/ |
There was a problem hiding this comment.
I think local-assets/ isn't referenced anywhere in the repo. Should we drop it, or is it a local workflow worth documenting?
| Multi.new() | ||
| # A section owns its section resources, but also points back to the root | ||
| # section resource. Clear that circular reference before deleting the | ||
| # section so the section_resources ON DELETE CASCADE can run safely. | ||
| |> Multi.update_all( | ||
| :clear_root_section_resource, | ||
| from(s in Section, where: s.id == ^section.id), | ||
| set: [root_section_resource_id: nil] | ||
| ) | ||
| |> Multi.delete(:section, section) | ||
| |> Repo.transaction() | ||
| |> case do | ||
| {:ok, %{section: deleted_section}} -> {:ok, deleted_section} | ||
| {:error, :section, changeset, _changes} -> {:error, changeset} | ||
| {:error, _operation, reason, _changes} -> {:error, reason} | ||
| end |
There was a problem hiding this comment.
I tried it: with just Repo.delete(section) the spec goes green, the DB comes out clean, and the new test passes the same. Do you think it makes sense to keep the change, or should we revert it?
| section_resource_ids = | ||
| SectionResource | ||
| |> where([sr], sr.section_id == ^section.id) | ||
| |> select([sr], sr.id) | ||
| |> Oli.Repo.all() | ||
|
|
||
| assert section.root_section_resource_id in section_resource_ids | ||
|
|
||
| assert {:ok, %Section{}} = Sections.delete_section(section) | ||
| refute Sections.get_section_by_slug(section.slug) | ||
| refute Oli.Repo.exists?(from sr in SectionResource, where: sr.id in ^section_resource_ids) |
There was a problem hiding this comment.
I like these assertions, I'd keep them. Only thing: I ran them with and without the change and they pass the same, so for now they wouldn't quite cover the new bit.
| end | ||
| rescue | ||
| e in Ecto.ConstraintError -> | ||
| e in [Ecto.ConstraintError, Postgrex.Error] -> |
There was a problem hiding this comment.
I think you added Postgrex.Error here but teardown_section didn't get it, and that one runs first. If it failed there the request would stop and it wouldn't get to delete the project. Was that intentional?
| project_sections = | ||
| from(s in Section, where: s.base_project_id == ^project.id) | ||
|
|
||
| result = | ||
| Multi.new() | ||
| # Full-course archives can contain delivery sections in addition to the | ||
| # section created by automation setup. Release every section owned by | ||
| # this disposable project before revision deletion cascades into its | ||
| # section_resources. | ||
| |> Multi.update_all( | ||
| :clear_project_section_roots, | ||
| project_sections, | ||
| set: [root_section_resource_id: nil] | ||
| ) | ||
| |> Multi.delete_all(:project_sections, project_sections) |
There was a problem hiding this comment.
I looked at the archive and didn't find sections inside it, so I'm not clear on when this cleanup would kick in. Could the case you hit have been a previous run leaving the section undeleted instead? If that's it the code still helps, but I'd tweak the comment.
| # Deleting a fully imported automation course can exceed Cowboy's default | ||
| # 60-second HTTP/1 idle timeout. Scope the longer timeout to this endpoint | ||
| # rather than holding every connection open longer application-wide. | ||
| defp extend_teardown_idle_timeout( | ||
| %Plug.Conn{adapter: {Plug.Cowboy.Conn, cowboy_request}} = conn | ||
| ) do | ||
| :cowboy_req.cast({:set_options, %{idle_timeout: 300_000}}, cowboy_request) | ||
| conn | ||
| end |
There was a problem hiding this comment.
I measured the teardown request across several runs and got around 3 seconds. The one getting close to the limit is setup, at 20–30. Had you hit a case where teardown hung? It may well have been slower before the other fixes in this PR.
| stale_automation_section = | ||
| insert(:section, | ||
| title: "Section imported with the project archive", | ||
| base_project: project | ||
| ) |
There was a problem hiding this comment.
Same point I left on automation_setup.ex: the title says the section came with the archive, but I didn't find sections inside the zip. If the real case is that it was left over from a previous run, I'd put that in the title — it's what whoever lands here in a year is going to read.
2 participants
Summary
Adds Playwright coverage for the BioBeyond Unit 7: Blue Planet → Our Blue Planet adaptive lesson.
The test imports the full course, navigates to the correct lesson, and completes all lesson screens twice:
100.A lesson-specific driver handles the lesson’s navigation and evaluation behavior without modifying the shared
AdaptiveDeckPOorAdaptiveHappyPathTaskhelpers. It also waits for fill-in-the-blank values to persist before submission and prevents actions from leaking into the following screen.Assets
The following assets must exist in the Playwright asset bucket:
How to run locally
Start Torus locally, then run the following from
assets/automation:The automation API key must be a valid locally generated key. Without it, Playwright will report the test as skipped.
video.webm
See: https://eliterate.atlassian.net/browse/MER-5670