Allow returning precompiled views without sources #2463
Conversation
|
Hi @Alxandr, I'm your friendly neighborhood Microsoft Open Technologies, Inc. Pull Request Bot (You can call me MSOTBOT). Thanks for your contribution! The agreement was validated by Microsoft Open Technologies, Inc. and real humans are currently evaluating your PR. TTYL, MSOTBOT; |
ghost
added
the
|
@pranavkm can you review? |
|
The change looks good although we should add a functional test to ensure this behaves correctly. See - https://github.com/aspnet/Mvc/blob/dev/test/Microsoft.AspNet.Mvc.FunctionalTests/PrecompilationTest.cs. The test would do something along the lines of deleting the |
|
My usecase was having the views in another project and precompiling them, though deleting the @pranavkm do you want me to add these tests? |
That should suffice - fewer test webapplications to manage. |
|
@pranavkm I've added unit tests, but I can't run them, because MVC doesn't build (it didn't build when I started making changes). |
|
@Alxandr our dev feeds are a bit busted at this point (should be good soon). In the meanwhile, you could try pointing to the volatile feed and retry building. Edit |
|
Ugh, dont point to the volatile feed. |
|
Well then, ignore my suggestion 😹 |
| } | ||
| finally | ||
| { | ||
| File.WriteAllText(Path.Combine(viewsDirectory, "Layout.cshtml"), layoutContent.TrimEnd(' ')); |
There was a problem hiding this comment.
What's with the trimming at the end?
|
BTW for the record I think it's weird for a test to delete files that are checked in, and to then attempt to write them back to disk... no? |
|
@Eilon I agree. We should probably revisit how these tests are done. We have other tests in this test area that edit files on disk and they are problematic to run in parallel (dnx451 + dnxcore50). |
|
|
||
| // Act - 2 | ||
| // Delete the Layout file and verify it is still served. | ||
| await DeleteFile(viewsDirectory, "Layout.cshtml"); |
There was a problem hiding this comment.
This doesn't actually cover the change you made in the code path. The first time the view is served, IsValidatedPreCompiled is set to true - so we don't use the code path that you made the change in. What I don't follow though is that we're serving views that have been deleted from disk after we added file watchers on it. That sounds like a bug and might be something we need to address.
What your test might need to do would be closer along the lines of
try
{
// Act
await DeleteFile(viewsDirectory, "_ViewStart.cshtml");
await DeleteFile(viewsDirectory, "_Layout.cshtml");
await DeleteFile(viewsDirectory, "Index.cshtml");
var response = await client.GetAsync("http://localhost/Home/Index");
// Assert
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
var parsedResponse1 = new ParsedResponse(responseContent);
Assert.StartsWith(assemblyNamePrefix, parsedResponse1.ViewStart);
Assert.StartsWith(assemblyNamePrefix, parsedResponse1.Layout);
Assert.StartsWith(assemblyNamePrefix, parsedResponse1.Index);There was a problem hiding this comment.
With regards to the watchers (if I'm reading this right), the source-files are never on disk to begin with (as far as MVC is concerned). They are precompiled in another project, so the paths MVC looks for them are non-existing. There is no Views directory at all.
There was a problem hiding this comment.
But they do exist in this case, right? Moving the deletion further up would more closely resemble your scenario?
|
⌚ |
|
I think it might be possible to inject a |
|
We do have unit tests for this type - https://github.com/aspnet/Mvc/blob/dev/test/Microsoft.AspNet.Mvc.Razor.Test/Compilation/CompilerCacheTest.cs - perhaps we can live with the absence of functional tests until we can sort out a plan for not doing disk IO. @Eilon thoughts? |
|
Doing disk IO might be fine if it's perhaps not editing files that are part of the repo. For example, can the test copy a folder with a sample site to another folder, do whatever it wants (e.g. edit files, delete files), then blow away the whole thing? And even put that whole folder in the |
|
Hi @Alxandr, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! TTYL, DNFBOT; |
|
Do I need to sign a new CLA? Anyways, should I revert the commit that added unit tests? |
|
@Alxandr, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
|
I think we can leave the functional test you had as is - it just needs to be fixed. See #2463 (comment). |
|
Turns out it's quite hard to try to fix this thing when I can never get Mvc to build on my machine. Any good commits I can use as a starting point? |
|
@Alxandr, what issues are you running into? The dev feed's been stable for some time now. |
|
Well, after deleting my Yet it crashes when trying to launch the xunit tests.
|
Alxandr
force-pushed
the
precompiled-razor-no-source
branch
from
987fc8e to
8d90016
Compare
|
Ok, so I got it compiling. I found a timezone (or similar) bug in MVC though, but I'll be reporting that in a separate issue. I also rebased off |
|
Looks good. I'll have this merged in. |
|
@Alxandr looks like this broke one of our existing tests (see AppVeyor) - DeletingPrecompiledGlobalFile_PriorToFirstRequestToAView_CausesViewToBeRecompiled. @Eilon, suggestions on what we should do here - the test is trying to verify a scenario that is contrary to the change @Alxandr made here. The scenario:
Should the view now recompile? Without the change, it does. |
|
@pranavkm logically that should work, no? If you change a dependency of a CSHTML file (e.g. by changing the global imports), the CSHTML file should recompile... |
|
@Alxandr we have a design meeting tomorrow afternoon to decide some open questions about precompilation. The other test might no longer be relevant once based on what we land on. I'll update this thread once we have a decision on this. |
|
@Alxandr if you'd like to attend this design discussion meeting I can send you a link to join us. I see you're in Norway so it might be a bit late for you though. The meeting is at 3pm Pacific Time, which is midnight for you. If you're interested, let me know and I'll send you an invite code. |
|
Midnight in 20 hrs, or midnight 4hrs ago? (sorry, but terms like "today" and "tomorrow" get especially confusing when operating on different timezones when it's around or just passed midnight ^^). |
|
Oh sorry 😄 It's at 3pm Pacific Time 5/12/2015, and 12am Norway Time 5/13/2015 (that is, very, very early on Wednesday). |
|
I sent you an invite in case you can attend. |
|
@Eilon, did you happen to reach a decision on this PR? |
|
@panesofglass yeah we had a meeting with several folks from the team, plus @Alxandr and we put down our notes here: #2551 If you'd like, please have a read through that and any extra notes/questions/comments you have. Right now the team is trying to wrap up the beta5 milestone, plus this weekend in the US is a long weekend (Sat-Sun-Mon off), and several folks are turning it into an extra-extra-long weekend (Fri-Tue off!), so I hope that later next week we can pick this up again and come up with a final design. |
|
This should be fixed as part of dfacd25. |
Resolves #2462