Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix zmount test cases #3608

Merged
merged 11 commits into from
Dec 18, 2024
Merged

Fix zmount test cases #3608

merged 11 commits into from
Dec 18, 2024

Conversation

samaradel
Copy link
Contributor

@samaradel samaradel commented Nov 10, 2024

Description

Fix zmount test suite expected errors.

Changes

image

Related Issues

Tested Scenarios

  • Update the value of size to get errors

Checklist

  • Tests included
  • Build pass
  • Documentation
  • Code format and docstrings
  • Screenshots/Video attached (needed for UI changes)


expect(result).toThrow();
expect(result).toBe(size.toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

can directly do expect(zmount.challenge()).toBe....


expect(result).toThrow();
expect(result).toBe(size.toString());
});

test("Max value for size.", () => {
const size = 100 * 1025 ** 4;
Copy link
Contributor

Choose a reason for hiding this comment

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

What do these numbers mean? try extracting to a const

max: "size must not be greater than 10995116277760",
},
}),
);
});

test("Size doesn't accept decimal value.", () => {
const size = 1.5;
Copy link
Contributor

Choose a reason for hiding this comment

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

is that 1.5 byte?

min: "size must not be less than 104857600",
},
}),
);
});

test("Size empty value.", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

While we are at it, check against null, undefined, NaN, {} , and []

}).toThrow(
expect.objectContaining({
constraints: {
max: "size must not be greater than 10995116277760",
Copy link
Contributor

Choose a reason for hiding this comment

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

check against the boundaries max, and min

and off by 1 min - 1 and max+1

Copy link
Contributor

@xmonader xmonader left a comment

Choose a reason for hiding this comment

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

We need to use better descriptions to describe the cases better

@samaradel samaradel marked this pull request as draft November 11, 2024 10:55
@samaradel samaradel marked this pull request as ready for review November 11, 2024 13:23
@samaradel samaradel requested a review from xmonader November 11, 2024 13:23
Copy link
Contributor

@0oM4R 0oM4R left a comment

Choose a reason for hiding this comment

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

passed locally, not sure about the tests description
image

@xmonader xmonader removed the request for review from ramezsaeed November 18, 2024 11:39
const result = () => zmount.challenge();
it("should fail validation if size is greater than the maximum", async () => {
// Greater than 10 TB
expect(() => (zmount.size = 15 * 1024 ** 4)).toThrow();
Copy link
Contributor

Choose a reason for hiding this comment

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

define the min and max as constants before any of the tests

@amiraabouhadid
Copy link
Contributor

please place the min and max values in a constant before any of the tests. Please move tests that check if size is null, Nan or not an int before the min and max tests

@samaradel samaradel marked this pull request as draft November 26, 2024 09:33
@samaradel samaradel marked this pull request as ready for review November 26, 2024 09:43
Copy link
Contributor

@AhmedHanafy725 AhmedHanafy725 left a comment

Choose a reason for hiding this comment

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

you should add a test for setting a floating number

Comment on lines 7 to 9
const minSize = 50 * 1024 ** 2; // Less than 100 MB
const maxSize = 15 * 1024 ** 4; // Greater than 10 TB
const validSize = 5 * 1024 ** 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

it's better to have a random size (within a certain range) to test with

you can use this method to generate the random number

function getRandomNumber(min: number, max: number): number {

let zmount: Zmount;
const minSize = 50 * 1024 ** 2; // Less than 100 MB
const maxSize = 15 * 1024 ** 4; // Greater than 10 TB
const validSize = 5 * 1024 ** 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we initializing this with a value, if we're using getRandomNumber


expect(result).toThrow();
const result = zmount.challenge();
expect(typeof result).toBe("string");
Copy link
Contributor

Choose a reason for hiding this comment

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

you can cast validSize to string and check against its value

@samaradel samaradel marked this pull request as draft December 15, 2024 09:38
@samaradel samaradel marked this pull request as ready for review December 15, 2024 09:41
@AhmedHanafy725 AhmedHanafy725 merged commit bc6633b into development Dec 18, 2024
9 checks passed
@AhmedHanafy725 AhmedHanafy725 deleted the development_zmount branch December 18, 2024 09:59
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.

7 participants