Skip to content

Conversation

@jasonmccreary
Copy link
Contributor

This pull request contains changes for migrating your test suite from PHPUnit to Pest automated by the Pest Converter.

Before merging, you need to:

  • Checkout the shift-49229 branch
  • Review all of the comments below for additional changes
  • Run composer update to install Pest with your dependencies
  • Run vendor/bin/pest to verify the conversion

@jasonmccreary
Copy link
Contributor Author

⚠️ Shift detected helper methods in the following PHPUnit tests. Currently Pest does not support namespaces. As such, these functions may result in a naming conflict.

While Shift converted these methods to functions in the new Pest test, you should review these files to see if these functions may be inlined or extracted to a custom helper.

  • tests/Feature/AdminTest.php
  • tests/Feature/AuthTest.php
  • tests/Feature/SettingsTest.php
  • tests/Integration/Models/ThreadTest.php
  • tests/Integration/Models/UserTest.php

@jasonmccreary
Copy link
Contributor Author

⚗️ This Shift is still being refined. Please report any issues or suggestions to shift@laravelshift.com. Your feedback is what helps improve the experience for everyone.

@jasonmccreary
Copy link
Contributor Author

@driesvints the unit and integration tests ran successfully. However, I noticed this is still running BrowserKit. Not sure if Pest supports that. I would guess not. Might be a good time to convert to Dusk or HTTP Tests. 👍

I also noticed Livewire, which may have some of its own steps for Pest integration.

{
$user = User::factory()->create(['name' => 'Freek Murze', 'banned_at' => Carbon::now()]);

$this->put('/admin/users/'.$user->username().'/unban')
Copy link

@nunomaduro nunomaduro Sep 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On helpers, the $this->put needs to be replaced by test()->put.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I'll definitely update this in the Pest Converter.

@joedixon
Copy link
Contributor

Hey @jasonmccreary 👋

I got all the tests passing locally. Any chance you can give me permission to push to this branch?

Thanks!

@jasonmccreary
Copy link
Contributor Author

@joedixon, sorry, I'm not sure what you mean. The "allow maintainers to edit" is enabled. What else is there?

@joedixon
Copy link
Contributor

No idea what was going on there @jasonmccreary. Yesterday evening I was getting permissions errors and today all is well in the world. Thanks anyway.

@joedixon
Copy link
Contributor

I got the tests passsing @driesvints, but I haven't swapped out any of the BrowserKit tests for HTTP tests.

Are you happy if we get this merged and an issue for removing BrowserKit to keep the PR focused?

@jasonmccreary
Copy link
Contributor Author

Thanks all. I make some notes of the manual changes, but I believe some of these have already been tweaked based on some feedback from Freek as well.

@driesvints
Copy link
Member

Are you happy if we get this merged and an issue for removing BrowserKit to keep the PR focused?

@joedixon Yes definitely! Thanks : )

but I believe some of these have already been tweaked based on some feedback from Freek as well.

@jasonmccreary do you think it's useful for you if we did a new run of this shift to see if that fixes everything?

@jasonmccreary
Copy link
Contributor Author

@driesvints, nah, that's okay. I feel I got everything from the additional commits. No need for you guys to rework stuff. 👍

Copy link
Member

@driesvints driesvints left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the TestCase class still need to be abstract?

Can you send in a separate PR to remove the HttpAssertions trait? The assertForbidden method is now on the Laravel TestResponse class.

@joedixon joedixon requested a review from driesvints October 6, 2021 19:47
@driesvints driesvints merged commit deda96a into laravelio:main Oct 7, 2021
@driesvints
Copy link
Member

Thanks a bunch for this @jasonmccreary & @joedixon :)

@jasonmccreary jasonmccreary deleted the shift-49229 branch October 7, 2021 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants