Copilot commented on code in PR #6738:
URL: https://github.com/apache/texera/pull/6738#discussion_r3627049763
##########
frontend/src/app/common/util/computing-unit.util.spec.ts:
##########
@@ -36,6 +39,36 @@ import {
buildLocalComputingUnitUri,
} from "./computing-unit.util";
+function makeUnit(overrides: Partial<DashboardWorkflowComputingUnit> = {}):
DashboardWorkflowComputingUnit {
+ return {
+ computingUnit: {
+ cuid: 1,
+ uid: 1,
+ name: "unit-a",
+ creationTime: 1700000000000,
+ terminateTime: undefined,
+ type: "kubernetes",
+ uri: "http://localhost/wsapi";,
+ resource: {
+ cpuLimit: "1000m",
+ memoryLimit: "2Gi",
+ gpuLimit: "1",
+ jvmMemorySize: "2G",
+ shmSize: "64Mi",
+ nodeAddresses: [],
+ },
+ ...(overrides.computingUnit ?? {}),
+ },
+ status: "Running",
+ metrics: { cpuUsage: "N/A", memoryUsage: "N/A" },
+ isOwner: true,
+ accessPrivilege: "READ",
+ ownerGoogleAvatar: "",
+ ownerName: "owner",
+ ...overrides,
Review Comment:
`makeUnit` merges `overrides.computingUnit` into the nested `computingUnit`
object, but the final `...overrides` spread can overwrite `computingUnit` again
(replacing the merged object with a partial). This makes the helper unsafe if a
future test passes `computingUnit` overrides and can silently produce invalid
fixtures.
##########
frontend/src/app/common/util/computing-unit.util.spec.ts:
##########
@@ -239,3 +272,127 @@ describe("buildLocalComputingUnitUri", () => {
);
});
});
+
+describe("cpuResourceConversion (fallback units)", () => {
+ it("should fall back to the millicore scale for an unknown source unit", ()
=> {
+ // "k" is not a known CPU unit, so cpuScales[from] is undefined and the
"m" scale is used
+ expect(cpuResourceConversion("5k", "m")).toBe("5.00");
+ });
+
+ it("should fall back to the core scale for an unknown target unit", () => {
+ // "x" is not a known CPU unit, so cpuScales[to] is undefined and the ""
(cores) scale is used
+ expect(cpuResourceConversion("2", "x")).toBe("2");
+ });
+});
+
+describe("memoryResourceConversion (edge branches)", () => {
+ it("should treat a unitless source value as bytes", () => {
+ expect(memoryResourceConversion("1073741824", "Gi")).toBe("1.0000");
+ });
+
+ it("should treat a unitless target unit as bytes", () => {
+ expect(memoryResourceConversion("1Gi", "")).toBe("1073741824.0000");
+ });
+
+ it("should fall back to a byte scale for an unknown source unit", () => {
+ // "Ti" is not a known memory unit, so memoryScales[from] is undefined and
1 (bytes) is used
+ expect(memoryResourceConversion("5Ti", "")).toBe("5.0000");
+ });
+
+ it("should fall back to a byte scale for an unknown target unit", () => {
+ // "Ti" is not a known memory unit, so memoryScales[to] is undefined and 1
(bytes) is used
+ expect(memoryResourceConversion("1Gi", "Ti")).toBe("1073741824.0000");
+ });
+});
+
+describe("memoryPercentage (limit guard)", () => {
+ it("should return 0 when the limit is not positive", () => {
+ expect(memoryPercentage("512Mi", "0")).toBe(0);
+ });
+});
+
+describe("isComputingUnitShmTooLarge (non-Gi selected memory)", () => {
+ it("should treat a Mi selected memory as-is when comparing", () => {
+ expect(isComputingUnitShmTooLarge("2048Mi", 3, "Gi")).toBe(true);
+ expect(isComputingUnitShmTooLarge("2048Mi", 1024, "Mi")).toBe(false);
+ });
+});
+
+describe("getJvmMemorySliderConfig (memory unit handling)", () => {
+ it("should convert a Mi memory value to whole GiB", () => {
+ // 2048Mi -> 2Gi -> small path (<=3), defaultValue 2 (not 1)
+ const config = getJvmMemorySliderConfig("2048Mi");
+ expect(config.showJvmMemorySlider).toBe(false);
+ expect(config.jvmMemorySteps).toEqual([1, 2]);
+ expect(config.selectedJvmMemorySize).toBe("2G");
+ expect(config.jvmMemorySliderValue).toBe(1);
+ });
+
+ it("should floor sub-GiB Mi values up to a minimum of 1 GiB", () => {
+ // 512Mi -> floor(512/1024)=0 -> Math.max(1, 0) = 1
+ const config = getJvmMemorySliderConfig("512Mi");
+ expect(config.jvmMemorySteps).toEqual([1]);
+ expect(config.selectedJvmMemorySize).toBe("1G");
+ });
+
+ it("should build slider steps from a larger Mi value", () => {
+ // 8192Mi -> 8Gi -> slider path
+ const config = getJvmMemorySliderConfig("8192Mi");
+ expect(config.showJvmMemorySlider).toBe(true);
+ expect(config.jvmMemorySteps).toEqual([2, 4, 8]);
+ expect(config.selectedJvmMemorySize).toBe("2G");
+ });
+
+ it("should default to 1 GiB for a value with an unrecognized memory unit",
() => {
+ // "Ki" is neither Gi nor Mi, so memoryToGb falls back to 1
+ const config = getJvmMemorySliderConfig("500Ki");
+ expect(config.jvmMemorySteps).toEqual([1]);
+ expect(config.selectedJvmMemorySize).toBe("1G");
+ });
+});
+
+describe("ComputingUnitMetadataComponent", () => {
+ let fixture: ComponentFixture<ComputingUnitMetadataComponent> | undefined;
+
+ function render(unit: DashboardWorkflowComputingUnit):
ComputingUnitMetadataComponent {
+ TestBed.configureTestingModule({
+ declarations: [ComputingUnitMetadataComponent],
+ providers: [{ provide: NZ_MODAL_DATA, useValue: unit }],
+ });
+ fixture = TestBed.createComponent(ComputingUnitMetadataComponent);
+ fixture.detectChanges();
+ return fixture.componentInstance;
+ }
+
+ afterEach(() => {
+ fixture?.destroy();
+ fixture = undefined;
+ TestBed.resetTestingModule();
+ });
+
+ it("should expose the injected unit and a formatted creation time", () => {
+ const unit = makeUnit();
+ const instance = render(unit);
+ expect(instance.unit).toBe(unit);
+ expect(instance.createdAt).toBe(new
Date(unit.computingUnit.creationTime).toLocaleString());
+ });
+
+ it("should render owner access and the gpu limit when present", () => {
+ const unit = makeUnit({ isOwner: true });
+ unit.computingUnit.resource.gpuLimit = "2";
+ render(unit);
+ const text = (fixture as
ComponentFixture<ComputingUnitMetadataComponent>).nativeElement.textContent as
string;
+ expect(text).toContain("Owner");
+ expect(text).toContain("2");
+ expect(text).not.toContain("None");
+ });
+
+ it('should render the access privilege and "None" when there is no gpu
limit', () => {
+ const unit = makeUnit({ isOwner: false, accessPrivilege: "WRITE" });
+ unit.computingUnit.resource.gpuLimit = "";
+ render(unit);
+ const text = (fixture as
ComponentFixture<ComputingUnitMetadataComponent>).nativeElement.textContent as
string;
+ expect(text).toContain("WRITE");
+ expect(text).toContain("None");
+ });
Review Comment:
These component-rendering assertions are too broad: `textContent` contains
many other "2" substrings (e.g., `2Gi`, `2G`), so the test can pass even if the
GPU limit cell is wrong or missing. It’s more robust to assert against the
specific table row/cell for "GPU Limit" and "Access".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
