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

Strips the base off the generated URL from the AuthComponent. #1451

Merged
merged 1 commit into from Jul 26, 2013
Merged

Strips the base off the generated URL from the AuthComponent. #1451

merged 1 commit into from Jul 26, 2013

Conversation

Phally
Copy link
Contributor

@Phally Phally commented Jul 26, 2013

@jippi
Copy link
Contributor

jippi commented Jul 26, 2013

👍

@@ -1269,6 +1269,39 @@ public function testRedirectSessionReadEqualToLoginAction() {
}

/**
* test that the base is handled correctly.
Copy link
Member

Choose a reason for hiding this comment

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

"correctly" is a bit too general for my taste, why not explain it slightly more? A few keywords, even a sa list in the long description of the block would be fine.

Additionally/Alternatively you could also link to the lighthouse ticket:
https://cakephp.lighthouseapp.com/projects/42648-cakephp/milestones

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 agree, the docblock is useless. To be honest, I never use docblocks in my tests. Usually I make the method name as descriptive as possible. Then I will be able to easily comment out all the other tests, so that the tests run faster and output is limited. I don't see how a docblock helps a test, because the test tests usage of something.

Anyway, I'll put something better there. I am not going to put the link to the ticket there. Links die.

Copy link
Contributor

Choose a reason for hiding this comment

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

it's a small test, so it's fairly easy to scan - the real value of test docblocks is when things get really hairy, or the problem is really complex. This test is so simple it should be easy for everyone to scan and understand in conjunction with the test method name imo

Copy link
Member

Choose a reason for hiding this comment

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

Oh sh** gave you the wrong link:
https://cakephp.lighthouseapp.com/projects/42648/tickets/3922-authcomponentredirecturl-prepends-appbaseurl

Guess you understood anyway ;-)

I would do the opposite even though the true fact that links die.
The point of linking the tests to a ticket is to keep "the history" of things together.

We already have a few tests with which are linked to lighthouse.
I don't know if there is a official policy about that though...

Copy link
Member

Choose a reason for hiding this comment

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

The commit message already has the ticket number "Fixes #3922" so one get the url if required.

@ADmad
Copy link
Member

ADmad commented Jul 26, 2013

👍

jippi added a commit that referenced this pull request Jul 26, 2013
Strips the base off the generated URL from the AuthComponent.
@jippi jippi merged commit b345a8f into cakephp:master Jul 26, 2013
@Phally Phally deleted the master-3922 branch July 26, 2013 13:25
*
* @see https://cakephp.lighthouseapp.com/projects/42648/tickets/3922-authcomponentredirecturl-prepends-appbaseurl
*
* @return void This test method doesn't return anything.
Copy link
Member

Choose a reason for hiding this comment

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

bigsmile

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 aim to please.

@markstory
Copy link
Member

Thanks for taking care of this 👍

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

5 participants