Skip to content

Conversation

@Joshua-Lester3
Copy link
Collaborator

Description

Adds api controller to access the code listings pulled from the private nuget feed. Added try .NET functionality to access IntelliTect Try Containerapp and added interactive code execution window.

Ensure that your pull request has followed all the steps below:

  • Code compilation
  • Created tests which fail without the change (if possible)
  • All tests passing
  • Extended the README / documentation, if necessary

- Updated appsettings.json to include TryDotNet origin configuration.
- Introduced code-runner.css for styling the interactive code execution panel.
- Implemented trydotnet-module.js for managing TryDotNet functionality, including session management, code execution, and error handling.
- Enhanced site.js to initialize TryDotNet functionality alongside chat widget.
<PackageReference Include="Microsoft.AspNetCore.Mvc.Testing" />
<PackageReference Include="Microsoft.EntityFrameworkCore.Sqlite" />
<PackageReference Include="Microsoft.NET.Test.Sdk" />
<PackageReference Include="Moq" />
Copy link
Member

Choose a reason for hiding this comment

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

@Keboo is always sad to not see AutoMoq ;)

HttpClient client = factory.CreateClient();

// Act
using HttpResponseMessage response = await client.GetAsync("/api/ListingSourceCode/1");
Copy link
Member

Choose a reason for hiding this comment

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

The api seems a bit arbitrary from the outside of /1/1 for example, maybe more like /chapter/1/listing/3 is clearer? @Keboo thoughts on this? Maybe it's not necessary since we only use it and it's the advent of ai

Copy link
Member

Choose a reason for hiding this comment

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

Yea I think adding in a bit more context into the URL would be nice, just from a debugging perspective when looking at the logs.

/// <summary>
/// Minimal IWebHostEnvironment implementation that redirects ContentRoot to the TestData directory.
/// </summary>
internal sealed class TestWebHostEnvironment : IWebHostEnvironment
Copy link
Member

Choose a reason for hiding this comment

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

Is this difficult to plug into our existing testing WebHost?

Copy link
Member

Choose a reason for hiding this comment

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

Also a bit curious why we need this.


public async Task<ListingSourceCodeResponse?> GetListingAsync(int chapterNumber, int listingNumber)
{
string chapterDirectory = $"ListingSourceCode/src/Chapter{chapterNumber:D2}";
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's not an big issue, but are these files being copied to www directory? Do we really want the listings there if so?

Comment on lines +47 to +52
ServiceDescriptor? listingDescriptor = services.SingleOrDefault(
d => d.ServiceType == typeof(IListingSourceCodeService));
if (listingDescriptor != null)
{
services.Remove(listingDescriptor);
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ServiceDescriptor? listingDescriptor = services.SingleOrDefault(
d => d.ServiceType == typeof(IListingSourceCodeService));
if (listingDescriptor != null)
{
services.Remove(listingDescriptor);
}
builder.Services.RemoveAll<IListingSourceCodeService>();

/// <summary>
/// Minimal IWebHostEnvironment implementation that redirects ContentRoot to the TestData directory.
/// </summary>
internal sealed class TestWebHostEnvironment : IWebHostEnvironment
Copy link
Member

Choose a reason for hiding this comment

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

Also a bit curious why we need this.

@@ -0,0 +1,9 @@
namespace EssentialCSharp.Web.Models;

public class ListingSourceCodeResponse
Copy link
Member

Choose a reason for hiding this comment

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

nit: This looks like it might be a good place to use a record class

HttpClient client = factory.CreateClient();

// Act
using HttpResponseMessage response = await client.GetAsync("/api/ListingSourceCode/1");
Copy link
Member

Choose a reason for hiding this comment

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

Yea I think adding in a bit more context into the URL would be nice, just from a debugging perspective when looking at the logs.

Comment on lines +125 to +131
var mockWebHostEnvironment = new Mock<IWebHostEnvironment>();
mockWebHostEnvironment.Setup(m => m.ContentRootPath).Returns(testDataRoot);
mockWebHostEnvironment.Setup(m => m.ContentRootFileProvider).Returns(new PhysicalFileProvider(testDataRoot));

var mockLogger = new Mock<ILogger<ListingSourceCodeService>>();

return new ListingSourceCodeService(mockWebHostEnvironment.Object, mockLogger.Object);
Copy link
Member

Choose a reason for hiding this comment

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

This is a case where Moq.AutoMock would be helpful as it would decouple this from the constructor. Let me know if you struggle to use it.

Comment on lines +134 to +146
private static string GetTestDataPath()
{
// Get the test project directory and navigate to TestData folder
string currentDirectory = Directory.GetCurrentDirectory();
string testDataPath = Path.Combine(currentDirectory, "TestData");

if (!Directory.Exists(testDataPath))
{
throw new InvalidOperationException($"TestData directory not found at: {testDataPath}");
}

return testDataPath;
}
Copy link
Member

Choose a reason for hiding this comment

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

A couple things here. When working with the file system it is better to return the stronger typed objects rather than strings. So in this case returning DirectoryInfo rather than a string.

I am a little curious how Directory.GetCurrentDirectory() makes an assumption on the working directory where the tests are run (consider running dotnet test from the test project's directory rather than dotnet test from the solution directory).

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.

3 participants