Regression in 0.11.4rc1
Remy Blank
http://trac.edgewall.org/ticket/8153
The symptoms are:
- Mainnav entries redirected through [mainnav] with a relative .href
don't work (most ironical example: the "New ticket" link on t.e.o).
- Redirects for URLs with trailing slashes don't work anymore (e.g.
/newticket/ redirects to //newticket instead of /newticket, which leads
to a "page not found" error with tracd).
The regression was introduced in [7930] (yeah, I know, me again). I have
attached a patch to the ticket that fixes both issues.
IMO this is serious enough to warrant a second release candidate, so I
suggest we apply the patch and go for another round of testing.
Opinions?
-- Remy
anatoly techtonik
That suxx. That really suxx, because in the current state nobody is
able to file new tickets against current release candidate. No
surprise there were such negative comment in the ticket. I wonder if
it is possible to introduce web tests or functional tests for such
cases, because it appears that unit tests are useless here and give a
false sense of security.
--
--anatoly t.
Noah Kantrowitz
all for eating ones own dogfood, but that probably isn't the place.
--Noah
anatoly techtonik
think about how to improve or implement thorough web testing?
--anatoly t.
Noah Kantrowitz
If you want to add a test case for this particular thing, so write one
and submit it. We have a pretty big unit and functional testing
infrastructure. The point is that t.e.o isn't a test site and
shouldn't be treated as such.
--Noah
Remy Blank
> Could it be another motivation to double check before release
That was the whole point of doing release candidates.
> and
> think about how to improve or implement thorough web testing?
There are already a number of functional tests in the test suite, but we
could certainly use more. Writing good tests takes time, which is not
spent on fixing issues, so as usual there's a tradeoff here.
As for running non-released builds on t.e.o, I'm rather in favor. It was
a bit unfortunate that the "new ticket" link broke, as it does make
reporting the issue more challenging. OTOH, I wouldn't want t.e.o to run
trunk.
-- Remy
Jonas Borgström
Yes I think so. Or at least delay the release a few more days.
I can't understand how this managed stay undetected on t.e.o for almost
a full week. But since new tickets were somehow created daily I guess we
had no real reason to suspect anything.
Anyway, I've remeved the [mainnav] thing from t.e.o so the new ticket
links should be working again.
Remy, if you're happy with the patch you have so far please go ahead and
commit it.
/ Jonas
Jonas Borgström
> No, not sure how it would be ....
>
> If you want to add a test case for this particular thing, so write one
> and submit it. We have a pretty big unit and functional testing
> infrastructure. The point is that t.e.o isn't a test site and
> shouldn't be treated as such.
I disagree. t.e.o has always been used for release testing and this is
the first time it has had a major impact on the services t.e.o is
supposed to provide. So I think the benefits by far outweighs the risks.
/ Jonas
Jeroen Ruigrok van der Werven
>I disagree. t.e.o has always been used for release testing and this is
>the first time it has had a major impact on the services t.e.o is
>supposed to provide. So I think the benefits by far outweighs the risks.
Just to add my opinion:
I fully agree with your statement Jonas.
--
Jeroen Ruigrok van der Werven <asmodai(-at-)in-nomine.org> / asmodai
イェルーン ラウフロック ヴァン デル ウェルヴェン
http://www.in-nomine.org/ | http://www.rangaku.org/ | GPG: 2EAC625B
Think carefully of what you ask for, because you just may get it...
Christian Boos
> On 3/25/09 12:40 AM, Remy Blank wrote:
>
>> The following defect has just been reported:
>>
>> http://trac.edgewall.org/ticket/8153
>>
>> The symptoms are:
>>
>> - Mainnav entries redirected through [mainnav] with a relative .href
>> don't work (most ironical example: the "New ticket" link on t.e.o).
>>
>> - Redirects for URLs with trailing slashes don't work anymore (e.g.
>> /newticket/ redirects to //newticket instead of /newticket, which leads
>> to a "page not found" error with tracd).
>>
>> The regression was introduced in [7930] (yeah, I know, me again). I have
>> attached a patch to the ticket that fixes both issues.
>>
>> IMO this is serious enough to warrant a second release candidate, so I
>> suggest we apply the patch and go for another round of testing.
>>
>> Opinions?
>>
>
> Yes I think so. Or at least delay the release a few more days.
>
> I can't understand how this managed stay undetected on t.e.o for almost
> a full week. But since new tickets were somehow created daily I guess we
> had no real reason to suspect anything.
>
As you said, other tickets were created in the meantime, and no other
regression were found. But it's also OK for me if we release 0.11.4rc2
now and delay the actual release to the end of the week.
> Anyway, I've remeved the [mainnav] thing from t.e.o so the new ticket
> links should be working again.
>
> Remy, if you're happy with the patch you have so far please go ahead and
> commit it.
>
The href() semantic change could also have other unattended consequence,
for example for plugins. So maybe we should considerer reverting r7930
on the branch, keep it on trunk and mention it as an API change for 0.12
(TracDev/ApiChanges/0.12). Then either fix #8059 differently or consider
it fixed for 0.12 only.
-- Christian
Christian Boos
A release candidate means "we think that this version is good for
release". So it would even be contradictory to say so but not be
confident enough to run it on t.e.o. Even more, we periodically upgrade
t.e.o to latest of the stable branch, because the stable branch is just
that, supposed to be "stable". I see no reason to not continue to do
that. As jonas said, the benefits are greater than the risks here.
Besides, the risks are not that great - if something goes wrong, we just
fix it or, at worse, rollback a few revisions.
That being said, it would be nice if we could get some /real/ test
environments on edgewall.org.
Indeed, the current "official" demo site on hosted-projects.com has a
couple of problems:
- it runs 0.10.5 - who cares about this version anymore ;-)
- it's not spam protected
Ideally, the test environments should run the latest committed code (one
for 0.11-stable, another for 0.12dev). Maybe the new server is beefy
enough to allow those environments to run in a sandbox (chroot / vm)? If
not, then running at least the same version as the one used for t.e.o
itself should be doable without introducing additional security concerns.
-- Christian
Remy Blank
> The href() semantic change could also have other unattended consequence,
> for example for plugins.
True. The change affects constructs like:
href() + path
in the case where href() returned the empty string, that is, when the
Trac instance is at the root of a site. The patch fixes the only two
instances of this in 0.11-stable.
As another data point, a scan through all the plugins on trac-hacks
shows that the following plugins are affected:
authformplugin (for 0.9)
*authopenidplugin
autoqueryplugin
menusplugin
*openidplugin
personalreportsplugin
secsessionplugin (for 0.10)
The following could be affected:
dbauthplugin (for 0.9, 0.10)
revtreeplugin
The plugins marked with * also contain bad uses of href() that are fixed
by the new behavior, namely they previously set the "path" parameter of
the "trac_auth" cookie to the empty string instead of "/".
> So maybe we should considerer reverting r7930
> on the branch, keep it on trunk and mention it as an API change for 0.12
> (TracDev/ApiChanges/0.12). Then either fix #8059 differently or consider
> it fixed for 0.12 only.
At this point, I'm not sure anymore. The [7930] change makes the usage
of href more consistent, so I would suggest keeping it in the long run.
And if you think the transition should be made in 0.12, I'm ok with it.
So if there are no objections, I'll revert [7930] on 0.11-stable and fix
#8059 differently tonight. Then we could quickly discuss if the change
to Href makes sense or not, and either document it for 0.12 or revert it
on trunk as well.
-- Remy
Remy Blank
> So if there are no objections, I'll revert [7930] on 0.11-stable and fix
> #8059 differently tonight.
Done in [7988], all build slaves pass, and #8153 is fixed as well. So
from my side, we're ready for a second release candidate.
I'll merge the change to trunk as well, and I'll open a new ticket to
discuss the Href change.
-- Remy