Add `device_test` test helper

[kaspth]

Currently, to get device specific tests users have to wrap their tests in @@test_devices.each, which kinda spills out implementation details to apps.

Instead, we can add a device_test to abstract those details, which means apps get less indentation and less details they need to care about. See sign_out_for(display_details) now being able to just be sign_out, as well as the other helpers doing the same.

I've just updated two test cases to showcase the difference, I can update others if this is deemed a fit. I've also updated the Capybara selectors we've used.

[kaspth]

By letting device_name and display_details become instance state, this could also open the door to clearer abstractions ala:

  def device
    @device ||= Device.new(device_name, **display_details)
  end

  class Device
    def initialize(name, resolution:, mobile:, high_dpi:)
      @name, @resolution, @mobile, @high_dpi = name, resolution, mobile, high_dpi
    end

    def mobile?
      @mobile
    end

    def scaled_resolutions
      @resolution.map { |pixel_count| pixel_count / scale }
    end

    def scale
      @high_dpi ? 2 : 1
    end
  end

[andrewculver]

100% here for this change, but the indentation change on the system test files is going to kill people when they're merging from the starter repo... I don't have a good solution for this at the moment, other than to put a useless begin ... end block around them for now to maintain the indentation level, or create a script that will automatically reindent people's tests (removing the @@ lines) before they merge this update. 😬 Will keep noodling on this.

[jagthedrummer]

I agree that this is a great change and that the indentation change is going to make for a really messy update. I'll also try to think about solutions for that problem. 🤔

[kaspth]

Here's what I'm thinking for the update sequence:

1. I can remove the changes to the specific test file here, then users have got the device_test helper and they can update manually.
2. Then I can add a script to rewrite and reindent the tests that users can call when they want to. 3. We could use the script to update the starter repo tests and give users a month to run the script on their end. Then any resulting diff should hopefully be easy to sort through.

@andrewculver @jagthedrummer how does this sound to you?

I'm not sure where we'd announce these changes to users, just in the commit message? I could add a Action: prefix to indicate that we require an action from them. Sound good?

[jagthedrummer]

@kaspth That sounds like a reasonable approach to me. Figuring out where/how to announce it is a good question. Maybe we add a temporary section to the upgrade guide that mentions it?

[kaspth]

@jagthedrummer what's the upgrading guide, this one? https://bullettrain.co/docs/upgrades — that seems to target the surface level, but ideally we'd want something more like a changelog.

[jagthedrummer]

@kaspth Yep, that's the upgrade guide I was thinking of. I agree that a proper changelog would be a good thing to introduce.

[kaspth]

@jagthedrummer I'm going to be working on the update script for this now. Now that you've setup GitHub releases for Bullet Train, how do I use that to communicate what apps need to do?

[kaspth]

Added a script, but my system tests fail locally, I have to go, but I'll come back and run them on CI later.

[jagthedrummer]

Now that you've setup GitHub releases for Bullet Train, how do I use that to communicate what apps need to do?

I'm not 100% sure on the answer to that question, but here's my first stab at thinking through it.

Our ultimate goal isn't so much to land this PR (which is definitely a good and helpful PR, don't get me wrong). The ultimate goal is to be able to land a different PR that will consist of updating a bunch of tests to be simpler.

We want to take test files that look like this:

  @@test_devices.each do |device_name, display_details|
    test "user can so something on a #{device_name}" do
      resize_for(display_details)
      # actual tests here
    end
  end

And make them look like this:

  test "user can do something on a #{device_name}" do
    # actual tests here
  end

Specifically we want to:

• Remove the enclosing @@test_devices.each do block
• Remove one level of indentation from test inside those blocks
• Remove the resize_for(display_details) from tests in those blocks

(Do we also want to be able to remove on a #{device_name} from the description?)

So, I think that this PR is safe to merge at any time because it won't force a change on anyone.

I think we probably want to merge it and have it "in circulation" for a while before trying to land the follow-on PR, and I think that PR will be the one that needs some good messaging.

The other PR that makes all of those simplifications has the possibility of causing hairy merge conflicts. So we're pre-shipping a script that will help people side-step those conflicts. And that's the PR that will require some finesse and good communication.

For that other PR I think we'll want to:

• Identify the target version of BT that we want it to land in. Possibly a major version bump? Let's call it 2.0.0 for now, just for illustration.
• Update the upgrade docs to mention that there's a big change in going from 1.3.X to 2.0.0. And that the way to navigate the change is to:
  • Upgrade to the last-available 1.3.x.
  • Make sure everything is good and tests are passing
  • Run bin/updates/system_tests/use_device_test
  • Make sure everything is good and tests are passing
  • Commit the changes made by bin/updates/system_tests/use_device_test
  • Upgrade to 2.0.0
  • Hopefully not have any hairy merge conflicts due to indentation changes

As to how to use our new release process specifically to communicate this stuff, I'm not entirely sure. Release notes are auto-generated based on what's been merged, but we can always go in and edit them to add additional info. (Example release notes for 1.3.22.)

It might be good to have some sort of warning in the title of the PR that's going to update those tests. Something like [Potentially Breaking Change] Update system tests to remove 'test_devices.each'. Just something to maybe get someone's attention if they're skimming the PRs that went into that release.

What do you think, @kaspth? Does this sound generally like the right thing to do?

[kaspth]

Our ultimate goal isn't so much to land this PR (which is definitely a good and helpful PR, don't get me wrong). The ultimate goal is to be able to land a different PR that will consist of updating a bunch of tests to be simpler.

Yes, we're shipping an update script in here that Bullet Train apps can use so they can independently have their tests updated first. Then they won't see a ton of merge conflicts after we've run the update script and they pull commits.

And make them look like this:

We need to use device_test too. The script I've added already converts those examples you listed, and changes the code to:

-  @@test_devices.each do |device_name, display_details|
-    test "user can so something on a #{device_name}" do
-      resize_for(display_details)
-      # actual tests here
-    end
-  end
+  device_test "user can do something" do
+    # actual test code
+  end

So yes, I'm also removing on a #{device_name} since device_test inserts that.

So, I think that this PR is safe to merge at any time because it won't force a change on anyone.

I think we probably want to merge it and have it "in circulation" for a while before trying to land the follow-on PR, and I think that PR will be the one that needs some good messaging.

The other PR that makes all of those simplifications has the possibility of causing hairy merge conflicts. So we're pre-shipping a script that will help people side-step those conflicts. And that's the PR that will require some finesse and good communication.

I think this PR needs the messaging in it too, since it's easier to communicate alongside the code. E.g. what we expect of people and what. I'm still in favor of prefixing the merge commit with [Action Required].

Ok, I think I have an idea of what to do based on your release process idea here. Let me push some stuff and then I'll explain.

[kaspth]

@jagthedrummer ok, I've prepped the v2.0.0 milestone PR over here https://github.com/bullet-train-co/bullet_train/pull/964, it'll update to target main after we merge this. We can always change the milestone too.

Once I see CI passing with rerunning tests after running the script, I'll squash the commits and try to give some more info to apps in the commit message. I'll also link up the #964 in there so apps know what it looks like when they run the script.

[kaspth]

Ok, looks like something is going on with CI. I'll take a look.

[kaspth]

@jagthedrummer how does the commit message in 4f1b44c, explaining what apps need to do, sound to you?

[jagthedrummer]

@kaspth in general I think that description is headed in the right direction.

But I really don't think that we want to encourage people to run the script until they're ready to upgrade to whatever version has the modified versions of the tests in the starter repo.

Like, imagine this scenario:

• We merge this PR and release it
• A dev updates and runs the script, fixing their local tests
• Before the big 2.0.0 release we need to modify test/system/account_test.rb
• The dev updates to that new version

I think that they'd potentially end up with a merge conflict in test/system/account_test.rb (depending on the nature of the change we had to make). Or at least the changes would seem weird because indentation levels would be off.

If I'm right about that, then I think we don't want to encourage people to run that script until they're doing it as a pre-upgrade step for the 2.0.0 release. And if we mention it at all before then, we probably want to encourage people not to run it. And we definitely don't want to tag this PR with [Action Required]. (Again, that's if I'm right about the potential for merge conflicts coming from the other direction.)

[jagthedrummer]

I just realized that I don't think there's really any reason to have this out in the wild for a while. Previously we'd discussed doing that, but it was before we'd started explicitly versioning the starter repo.

But since we have started versioning it we could just sit on this PR until we're almost ready for 2.0.0 (or whatever the version is going to be). Then we add it in the release immediately before 2.0.0 (let's call it 1.99.0) along with the [Action Required] prefix. And we also add a new section/page on the /docs site that talks about how to get over the hump to 2.0.0. That would explicitly say to update to 1.99.0, then run the script, then go to 2.0.0.

[kaspth]

I just realized that I don't think there's really any reason to have this out in the wild for a while. Previously we'd discussed doing that, but it was before we'd started explicitly versioning the starter repo.

I don't know when/if 2.0.0 is happening, and I don't think we should sit on this for potentially months. There's absolutely a concern about the upgrade burden involved and how that happens, but I don't know if we have to be this cautious?

Since we're on 1.3.x, I'd rather we decided on 1.4.0 or 1.5.0, that would follow a shifted SemVer akin to Rails' 6.0, 6.1 etc.

I'll see if I can figure out how to verify if there's going to be merge conflicts or not for your earlier comment.

[jagthedrummer]

@kaspth, yeah, sorry, I wasn't meaning to say that we should sit on this for months. I was just using 2.0.0 as a convenient shorthand to discuss the general concept. I figured that was better than trying to be super precise about exactly which version we expect this to land in. I think 1.4.0 is probably OK.

[kaspth]

@jagthedrummer ahh ok, got it! Yeah, that line ended up getting a little blurred for me as we kept discussing.

Ok, so it looks like this PR passes CI and we run the script, so this should be ready to go. I'm fine with waiting to ship this just before ~1.4.0 then. Do you want to coordinate the release of this when you want to cut ~1.4.0? I'm not sure why CI doesn't pass on #964, but I can prep that.

[jagthedrummer]

@kaspth, I think we can push for 1.4.0 to be fairly soon. The things that I think we should button up first are:

• Get #964 passing tests, which hopefully won't uncover any new complications. (Btw, the diff for #964 seems a little weird to me because it's showing that it wants to put things into this branch that are already in this branch. I would expect it to show only the changes in test/system/**.)
• I'll work on a fix for https://github.com/bullet-train-co/bullet_train-core/issues/470 which will give us a good place to add a call-out about the 1.3.x => 1.4.0 transition.
• We write up the transition process somewhere in the docs and link it in the call-out mentioned above. (I can also handle this.)
• We give people in Discord a heads up that the 1.4.0 release is coming. Are there other places we should also do this? Maybe ask Andrew to mention it on his various social accounts?
• We merge this PR (with the [Action Required] alert in the PR title)
• We release version 1.3.x in both core and starter
• We merge #964
• We release version 1.4.0 in both core and starter

Does that sound reasonable? Am I missing anything?

[jagthedrummer]

@kaspth, I just released 1.4.0 as the first release where we explicitly pin the version number of core gems in the Gemfile of the starter repo. So I'm thinking that we'll target 1.5.0 (maybe next week?) as when we'll release #964.

To update the list from above, and make it a checklist, I think the process for landing this and the associated PR are:

• [x] Get #964 passing tests, which hopefully won't uncover any new complications. (Btw, the diff for #964 seems a little weird to me because it's showing that it wants to put things into this branch that are already in this branch. I would expect it to show only the changes in test/system/**.)
• [x] I'll work on a fix for https://github.com/bullet-train-co/bullet_train-core/issues/470 which will give us a good place to add a call-out about the 1.4.x => 1.5.0 transition.
• [x] We write up the transition process somewhere in the docs and link it in the call-out mentioned above. (I can also handle this.)
• [ ] We give people in Discord a heads up that the 1.5.0 release is coming. Are there other places we should also do this? Maybe ask Andrew to mention it on his various social accounts?
• [ ] We merge this PR (with the [Action Required] alert in the PR title)
• [ ] We release version 1.4.x in both core and starter
• [ ] We merge #964
• [ ] We release version 1.5.0 in both core and starter

[kaspth]

@jagthedrummer this sounds good to me! I'll work on #964 (I'm also confused why it's not just the test/system/** changes in there).

We give people in Discord a heads up that the 1.5.0 release is coming. Are there other places we should also do this? Maybe ask Andrew to mention it on his various social accounts?

Sure yeah, we could ask Andrew about this on Discord.

[kaspth]

@jagthedrummer I got the tests passing on #964 there were some small fixes needed to the script and there were amended into our commit here. So we should be good to go on that.

[jagthedrummer]

@kaspth, excellent! I'm going to have a short day today and then will be out for a long weekend. I want to have plenty of time to get the merge & release for these right, so I'm going to leave them until next week. They'll be on the top of my list when I'm back.

[kaspth]

@jagthedrummer perfect, sounds good! Enjoy your long weekend!

[jagthedrummer]

@kaspth, I just had one more thought on this. In the case where someone merges this PR from upstream but does not run the update script, we should probably display a deprecation messages in the console or raise an error or something.

Maybe by having resize_for continue to be a "real" method that displays the deprecation message before calling resize_display?

Something like:

def resize_for(display_details)
  puts "--------------------------------------------------------".red
  puts "resize_for is deprecated.".red
  puts "please run the following command to update your tests:".red
  puts "./bin/updates/system_tests/use_device_test".red
  puts "--------------------------------------------------------".red
  resize_display(display_details)
end

[gazayas]

I'm late to the conversation and I know this one is on the verge of being merged, just wanted to mention that we also have the Scaffolding::BlockManipulator in case we wanted to write any scripts that update block indentation, etc. in the future.

Looking forward to using this new device_test setup in my own app!

[kaspth]

eyy nice! 🎉

@gazayas good point, I'll try to keep a note for that if we need it in the future 😄