Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check TELESCOPE_ENABLED instead of `runningUnitTests()` #436

Closed
wants to merge 3 commits into from

Conversation

@owenconti
Copy link

commented Nov 25, 2018

This is an attempt at Taylor's comment from here: #415 (comment)

This will allow users to enable and use Telescope for unit tests.

Telescope Changes

  • Replace $app->runningUnitTests() with config('telescope.enabled')
  • Update the migrations to check if the tables exist before attempting to create them. The reason for this change is because unit tests will often drop/re-migrate the database. However, if you configure Telescope to use a different database than your test database, the Telescope DB will not be dropped, but it will be re-migrated.

Application changes

  • Add new database connection for Telescope:
        'telescope' => [
            'driver' => 'sqlite',
            'database' => database_path('telescope.sqlite'),
            'prefix' => '',
        ],
  • Change my config/telescope.php file to use a new environment variable for the database connection: 'connection' => env('TELESCOPE_DB_CONNECTION', 'mysql'),
  • Add the new environment variable to my .env file: TELESCOPE_DB_CONNECTION=telescope
  • Add new sqlite database file: touch database/telescope.sqlite
  • Change the Telescope::filter closure. Calling $this->app->isLocal() inside the closure throws an exception which I couldn't figure out:

Change I made:

        $telescopeFilter = $this->app->isLocal() || $this->app->runningUnitTests();

        Telescope::filter(function (IncomingEntry $entry) use ($telescopeFilter) {
            if ($telescopeFilter) {
                return true;
            }
            
            return $entry->isReportableException() ||
                   $entry->isFailedJob() ||
                   $entry->isScheduledTask() ||
                   $entry->hasMonitoredTag();
        });

Exception when using $this->app->isLocal() inside the closure:

ReflectionException: Class env does not exist

/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Container/Container.php:779
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Container/Container.php:658
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Container/Container.php:609
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Foundation/Application.php:735
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Container/Container.php:1222
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Foundation/Application.php:495
/var/www/remoteauth-app/app/Providers/TelescopeServiceProvider.php:26
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Support/HigherOrderCollectionProxy.php:60
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Support/HigherOrderCollectionProxy.php:60
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Support/Collection.php:455
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Support/HigherOrderCollectionProxy.php:61
/var/www/telescope/src/Telescope.php:232
/var/www/telescope/src/Telescope.php:201
/var/www/telescope/src/Telescope.php:235
/var/www/telescope/src/Telescope.php:358
/var/www/telescope/src/Watchers/QueryWatcher.php:43
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Events/Dispatcher.php:360
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Events/Dispatcher.php:209
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Database/Connection.php:826
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Database/Connection.php:682
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Database/Connection.php:635
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Database/Connection.php:333
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Database/Connection.php:304
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Database/Schema/Builder.php:73
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Database/Migrations/DatabaseMigrationRepository.php:169
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Database/Migrations/Migrator.php:556
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Database/Console/Migrations/MigrateCommand.php:91
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Database/Console/Migrations/MigrateCommand.php:63
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Container/BoundMethod.php:29
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Container/BoundMethod.php:87
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Container/BoundMethod.php:31
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Container/Container.php:572
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Console/Command.php:183
/var/www/remoteauth-app/vendor/symfony/console/Command/Command.php:255
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Console/Command.php:170
/var/www/remoteauth-app/vendor/symfony/console/Application.php:886
/var/www/remoteauth-app/vendor/symfony/console/Application.php:262
/var/www/remoteauth-app/vendor/symfony/console/Application.php:145
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Console/Application.php:89
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Console/Application.php:188
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Foundation/Console/Kernel.php:250
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Foundation/Testing/PendingCommand.php:136
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Foundation/Testing/PendingCommand.php:218
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Foundation/Testing/Concerns/InteractsWithConsole.php:55
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Foundation/Testing/RefreshDatabase.php:40
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Foundation/Testing/RefreshDatabase.php:17
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Foundation/Testing/TestCase.php:104
/var/www/remoteauth-app/vendor/laravel/framework/src/Illuminate/Foundation/Testing/TestCase.php:71
/var/www/remoteauth-app/tests/TestCase.php:15

Unresolved issues/questions

  • This will enable Telescope during tests by default because Telescope is enabled by default here: 'enabled' => env('TELESCOPE_ENABLED', true),
  • If anyone knows why the ReflectionException: Class env does not exist exception is thrown when calling $this->app->isLocal() inside the Telescope filter, that would be nice to clean up too.

owenconti added some commits Nov 25, 2018

Update Telescope to check `telescope.enabled` instead of `runningUnit…
…Tests()`. Update migrations to check before creating new tables.
@paras-malhotra

This comment has been minimized.

Copy link
Contributor

commented Nov 25, 2018

I'm not sure what the migration files have to do with the goal at hand, would be helpful to explain why these need to change. Also, this change is incomplete. Simply removing runningUnitTests would have been fine since registerWatchers does not register if telescope isn't enabled. But this change makes the registerWatchers check useless.

Finally, this is a breaking change, so should possibly target master.

@owenconti

This comment has been minimized.

Copy link
Author

commented Nov 25, 2018

@paras-malhotra Migration file changes are described in the description:

Update the migrations to check if the tables exist before attempting to create them. The reason for this change is because unit tests will often drop/re-migrate the database. However, if you configure Telescope to use a different database than your test database, the Telescope DB will not be dropped, but it will be re-migrated.

I agree with the semi-redundant check in registerWatchers. If Telescope is disabled, should any of the static methods in start() be called?

I will update the target branch. Edit: master is 46 commits behind 1.0.

@owenconti owenconti changed the base branch from 1.0 to master Nov 25, 2018

@owenconti owenconti changed the base branch from master to 1.0 Nov 25, 2018

@specialtactics

This comment has been minimized.

Copy link

commented Nov 26, 2018

Love it !

@themsaid

This comment has been minimized.

Copy link
Member

commented Nov 26, 2018

No need to change the migrations, I've applied the change with the config value.

@themsaid themsaid closed this Nov 26, 2018

@owenconti

This comment has been minimized.

Copy link
Author

commented Nov 26, 2018

@themsaid without the migration changes, users will get errors when using Telescope for tests if they use a separate DB for Telescope (which they should, otherwise the Telescope entries will get lost with RefreshDatabase)

Do you have another way around that problem?

@deleugpn

This comment has been minimized.

Copy link
Contributor

commented Nov 26, 2018

Would be nice if Telescope didn't load migrations by default and we had to run the migrations ourselves as it's a bit annoying when we're using a dedicated database and we don't want to run all migrations mixed from the project into the telescope database.

@themsaid

This comment has been minimized.

Copy link
Member

commented Nov 26, 2018

@owenconti honestly I don't understand why would you run Telescope in unit tests, maybe you want to debug a certain test but not the entire suit. If you are debugging a certain test just comment the RefreshDatabase and bring it back when you're done.

@owenconti

This comment has been minimized.

Copy link
Author

commented Nov 26, 2018

@themsaid It makes it much much easier to debug logs, exceptions, and queries.

Say I make a refactor and want to re-run my suite which results in 5 failed tests. I can easily look at Telescope to debug without having to re-run anything or comment out anything.

I'm not sure what the harm is in wrapping table creations in hasTable? Another solution could be that the database Telescope uses manages its own migrations. That way if another database is used for Telescope, it would have its own migrations table separate from the user's application. I think that is a much larger change though, which is why I opted to wrap the table creations in the if statement.

@themsaid

This comment has been minimized.

Copy link
Member

commented Nov 26, 2018

User with ID 1 can be any users created in any of the tests in your suite, that's why I think it's not very useful to run telescope for the entire test suite. I don't think you can find workable information from this.

@paras-malhotra

This comment has been minimized.

Copy link
Contributor

commented Nov 26, 2018

@owenconti you have at least 2 options if you want to do this:

  1. You can use a separate telescope DB for test debugging and add the appropriate environment configs in your phpunit.xml file
  2. Instead of using the RefreshDatabase trait, you can call a custom function that doesn't drop the telescope database perhaps by using the --path param
@owenconti

This comment has been minimized.

Copy link
Author

commented Nov 26, 2018

@themsaid I think there's a misunderstanding somewhere. I'm not sure what the user ID has to do with anything.

I think it's a valid use case to inspect and debug your application, regardless of if the requests are made during tests or not. I used it all day yesterday on my forked version without any issue, and it helps make debugging much easier.

@paras-malhotra your first suggestion does not work because when Laravel runs the RefreshDatabase migration, the tables already exist in the Telescope database.

Your second suggestion is valid, but why make developers jump through those hoops when an easier approach for everyone is to add a safety check in your migration, or have the framework track migrations per connection?

@themsaid

This comment has been minimized.

Copy link
Member

commented Nov 26, 2018

Question is, how many developers really want to run Telescope in their test environment. I honestly don't find it very useful.

Anyway I'll look into tracking migrations on different connections in the morning, I believe if we find a better way to handle this on the framework level it will solve the issue you're facing with Telescope and tests.

@owenconti

This comment has been minimized.

Copy link
Author

commented Nov 26, 2018

@themsaid Thanks.

I agree that the issue is actually at the framework level for tracking migrations properly. I was just looking for an interim fix in the meantime.

@paras-malhotra

This comment has been minimized.

Copy link
Contributor

commented Nov 26, 2018

@owenconti Laravel does not drop all your tables/databases when running the RefreshDatabase, it only drops the tables/databases that are configured in your app and used in your migration files.

public function __construct()
{
$this->schema = Schema::connection(
config('telescope.storage.database.connection')
);
}

If you see the migration above, it switches to the database connection specified in config('telescope.storage.database.connection'). If you change this config using environment variables defined in your phpunit.xml file, Laravel will only drop those tables in the test Telescope database and your original Telescope database will stay intact.

Hope this makes sense now.

@owenconti

This comment has been minimized.

Copy link
Author

commented Dec 1, 2018

Hi @themsaid, any luck with tracking migrations per DB connection?

@themsaid

This comment has been minimized.

Copy link
Member

commented Dec 1, 2018

If you use --path and --database with the migrate command you can migrate isolate Telescope migrations in a specific connection.

@sagalbot

This comment has been minimized.

Copy link

commented Dec 3, 2018

honestly I don't understand why would you run Telescope in unit tests

@themsaid For what it's worth I think debugging tests with Telescope would be quite useful, especially when working with integration tests that make requests.

/**
 * @test
 */
public function it_lists_posts()
{
    $post = factory(Post::class)->create();
    $this->getJson('/api/posts')->assertStatus(200)->assertJson([$post->toArray()]);
}

☝️ I find myself writing tests like this one quite frequently with assertStatus(200) as my first assertion. It seems like the logical first assertion to make: "did the thing error out or redirect?".

However, debugging this from the terminal can be a pain. If it 500's: you get the '200 didn't match 500' test failure. From there, I either need to switch assertStatus to dump(), then scroll through to find the exception, or sift through logs. This is just one example where the terminal output doesn't help you find the real issue. Errors in migrations is another good one. Using Telescope to debug during testing would make hunting down those exceptions significantly easier.

@GuidoHendriks

This comment has been minimized.

Copy link

commented Dec 12, 2018

Question is, how many developers really want to run Telescope in their test environment. I honestly don't find it very useful. - @themsaid

What was the answer to this? Very interesting to see it's not only made possible but also enabled by default.

@themsaid

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

The way tests run will make it very hard for Telescope to operate, I'll need to dedicate some time to explore this but I don't think it's simple :)

@GuidoHendriks

This comment has been minimized.

Copy link

commented Dec 12, 2018

But since this change it's enabled by default for tests:
88d760a

Doesn't seem like a good default behavior. It caused my test suite to fail because it ran out of memory. Was pretty hard to find out what the issue was.

@themsaid

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

@GuidoHendriks some people use it while running a single test or a test class, problems start to appear when it's running while a large test suite is being run.

@GuidoHendriks

This comment has been minimized.

Copy link

commented Dec 12, 2018

@themsaid Doesn't feel like a good reason to change the default behavior in a patch, as it's a breaking change for people with large test suites. Allowing people to enable it for tests with a separate config value would make more sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.