Add unit tests for link tag helpers#2197
Conversation
c82ac66 to
32b81f7
Compare
This comment was marked as outdated.
This comment was marked as outdated.
32b81f7 to
4c16f80
Compare
|
So basically, if you want to use Also the output TagName needs to be set to --- a/TASVideos/TagHelpers/WikiLinkTagHelpers.cs
+++ b/TASVideos/TagHelpers/WikiLinkTagHelpers.cs
@@ -11,7 +11,9 @@ public class PubLinkTagHelper(IHtmlGenerator generator) : AnchorTagHelper(genera
public override void Process(TagHelperContext context, TagHelperOutput output)
{
- Page = $"/{Id}M";
+ output.TagName = "a";
+ Page = "/Publications/View";
+ RouteValues.Add("Id", Id.ToString());
base.Process(context, output);
}
}Though I'm wondering why we don't just set the href directly with |
4c16f80 to
e260781
Compare
|
Thank you, that was the small detail I was missing. And since it doesn't work for wiki pages, I've reverted the change to
The idea was to use the same mechanism as |
| public class AddLinkTagHelper(IHtmlGenerator generator) : AnchorTagHelper(generator) | ||
| { | ||
| public override void Process(TagHelperContext context, TagHelperOutput output) | ||
| => ProcessAsync(context, output).Wait(); |
There was a problem hiding this comment.
This is bad. Why are we doing this? .Wait() on async calls is asking for deadlocks
There was a problem hiding this comment.
The first commit is to fix an inconsistency in subclasses of AnchorTagHelper. In TagHelper, ProcessAsync calls Process, which is fine. AnchorTagHelper overrides Process only, which is fine. But those subclasses were overriding ProcessAsync only, so if Process is called (which I guess was never happening, thankfully) then it would be the implementation from AnchorTagHelper, which is obviously not going to produce the same output.
There was a problem hiding this comment.
Can we not implement both Process and ProcessAsync? The .Wait() is a no go for me, need another way
| { | ||
| output.TagName = "a"; | ||
| await base.ProcessAsync(context, output); | ||
| base.Process(context, output); |
There was a problem hiding this comment.
Similarly, why are we not doing async here?
There was a problem hiding this comment.
AnchorTagHelper doesn't override ProcessAsync, so that would actually be the implementation from TagHelper, which makes a virtual call to this.Process, which calls this method recursively.
e260781 to
05a9b68
Compare
|
Moved that to a separate PR, and rewrote the tests here to be async. |
| namespace TASVideos.TagHelpers; | ||
|
|
||
| public class PubLinkTagHelper : TagHelper | ||
| public class PubLinkTagHelper(IHtmlGenerator generator) : AnchorTagHelper(generator) |
There was a problem hiding this comment.
Yes?
Do you mean the constructor parameter? It's being passed through to the base class.
| { | ||
| output.TagName = "a"; | ||
| output.Attributes.Add("href", $"/{Id}M"); | ||
| Page = "/Publications/View"; |
There was a problem hiding this comment.
Wyy make this change? does it change any behaviors?
There was a problem hiding this comment.
My earlier comment:
The idea was to use the same mechanism as
<a asp-page/>. After this PR I wanted to add more link helpers, but existing calls mostly use that rather than<a href/>. Also, I'm hoping that<a asp-page/>would help with tracking page links (for generating referrer lists).
The unit tests should catch regressions, but I also did a bit of manual testing.
05a9b68 to
1fdb901
Compare
| if (virtualPath is not null) | ||
| { | ||
| // not sure why, but the inner Route adds the whole path again as `?path=`, so strip that | ||
| // TODO hmm, the queryparam reflects AnchorTagHelper.Page, while the actual path reflects the route template, meaning this is discarding info that should be asserted against |
There was a problem hiding this comment.
@YoshiRulz Are you going to do this TODO? We want complete PRs, not ones with work left to do.
There was a problem hiding this comment.
I've honestly forgotten what this was about, but based on my comment, the opportunity here is that there's a second string which could theoretically be exfiltrated to the calling unit test. I'm not sure how to do that given the architecture.
I believe this PR is complete modulo a merge conflict, just pending adelikat's review.
1fdb901 to
5b45a0a
Compare
5b45a0a to
ec9ebb3
Compare
Testing these:
tasvideos/TASVideos/TagHelpers/WikiLinkTagHelpers.cs
Lines 8 to 91 in ffe8999