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

[3.3][Tests] Extension coverage #6521

Merged
merged 6 commits into from
Mar 31, 2017
Merged

[3.3][Tests] Extension coverage #6521

merged 6 commits into from
Mar 31, 2017

Conversation

GwendolenLynch
Copy link
Contributor

No description provided.

@GwendolenLynch GwendolenLynch added this to the Bolt 3.2 - Feature release milestone Mar 27, 2017
@GwendolenLynch GwendolenLynch changed the base branch from release/3.2 to release/3.3 March 27, 2017 14:37
@GwendolenLynch GwendolenLynch changed the title [3.2][Tests] Extension coverage [3.3][Tests] Extension coverage Mar 27, 2017
*/
protected function registerServices(Application $app)
{
$this-> extendDatabaseSchemaServices();
Copy link
Member

Choose a reason for hiding this comment

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

derp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Derp?

Copy link
Member

Choose a reason for hiding this comment

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

The space

Copy link
Contributor Author

Choose a reason for hiding this comment

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

derp

parent::setUpBeforeClass();
$fs = new Filesystem();
$fs->copy(PHPUNIT_ROOT . '/resources/test.twig', PHPUNIT_WEBROOT . '/theme/base-2016/test.twig', true);
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you just add an Array_Loader to the Twig_Loader_Chain for that render test?
The less files we have to copy around the better.

@GwendolenLynch
Copy link
Contributor Author

@CarsonF: Updated

@CarsonF
Copy link
Member

CarsonF commented Mar 27, 2017

"Calback" <- missing second L.

Not thrilled about the defined constant usage but I'll let it slide.

You should also render with sandbox enabled so "safe" => true is asserted.

Is registerTwigPaths getting asserted?

@GwendolenLynch
Copy link
Contributor Author

GwendolenLynch commented Mar 27, 2017

"Calback" <- missing second L.

Fixed

Not thrilled about the defined constant usage but I'll let it slide.

Makes at least two of us

You should also render with sandbox enabled so "safe" => true is asserted.

Done

Is registerTwigPaths getting asserted?

Yes

@CarsonF
Copy link
Member

CarsonF commented Mar 27, 2017

Not thrilled about the defined constant usage but I'll let it slide.
Makes at least two of us

Why do you not want to use ArrayLoader then?

@GwendolenLynch
Copy link
Contributor Author

Why do you not want to use ArrayLoader then?

Fine, done 😜

{
return [
'koala',
'dropbear' => ['position' => 'prepend', 'namespace' => 'Marsupial'],
Copy link
Member

Choose a reason for hiding this comment

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

How is this getting tested?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A lack of exceptions

Copy link
Member

Choose a reason for hiding this comment

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

😞

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, but it is more than we have, and I have very little desire/allowance time to work on tests, and am trying to turn a few quick PRs over to move a few things along … with the hope it makes it easier for others to help.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

with the hope it makes it easier for others to help.

Plus Twig internals and writing tests, just sayin' 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I gave in, and added the tests … You owe me a merge now!

Filter dropbear {{ name|dropbear }}
TWIG;
$loader = new ArrayLoader(['marsupial.twig' => $template]);
$app['twig']->setLoader($loader);
Copy link
Member

Choose a reason for hiding this comment

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

Just also had the thought that you could set this with $app['twig.templates'] instead of creating an ArrayLoader directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, but I CBF iterating test to perfection … they're tests, better things to focus on.

Copy link
Member

@CarsonF CarsonF left a comment

Choose a reason for hiding this comment

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

I'm good with this now, if you are good with my commits.

@GwendolenLynch
Copy link
Contributor Author

I am … over to you @bobdenotter :shipit:

@CarsonF CarsonF modified the milestones: Bolt 3.3 - Feature release, Bolt 3.2 - Feature release Mar 31, 2017
@bobdenotter
Copy link
Member

:shipit: indeed!

@bobdenotter bobdenotter merged commit 6b5510c into bolt:release/3.3 Mar 31, 2017
@GwendolenLynch GwendolenLynch deleted the tests/extension-coverage branch March 31, 2017 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants