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

Add Qtest library that uses class constraints #2013

Merged
merged 3 commits into from
Nov 15, 2024
Merged

Add Qtest library that uses class constraints #2013

merged 3 commits into from
Nov 15, 2024

Conversation

sezna
Copy link
Contributor

@sezna sezna commented Nov 11, 2024

This PR introduces Qtest, a library for running Q# unit tests. It includes two namespaces: Functions for testing functions and Operations for testing operations. Operations also allows for qubit state preparation before tests.

To demonstrate, I converted MeasureSignedIntTests() in signed/src/Tests.qs to use Qtest. The result is fewer lines of code and clearer test intentions. Additionally, all tests run before reporting any failures, unlike the current behavior which stops at the first failure.

I haven't converted all existing Q# tests yet, as that's beyond this PR's scope. This PR aims to validate the API's utility before proceeding with further conversions.

Copy link
Contributor

@ScottCarda-MS ScottCarda-MS left a comment

Choose a reason for hiding this comment

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

Just one suggestion about adding some test cases. Other than that, looks good!

@Manvi-Agrawal
Copy link
Contributor

@sezna I am curious why are we having a QTest library in the new QDK instead of @Test attribute which was there in the classic QDK? Are we assuming that people are used to new QDK and classic QDK doesnt matter anymore?

@sezna
Copy link
Contributor Author

sezna commented Nov 12, 2024

Hey @Manvi-Agrawal, thanks for pointing that out. The classic QDK test mechanism was backed by Xunit and C#. The modern QDK has no connection to dotnet or Xunit. We do acknowledge that some things are different in the modern QDK from the classic QDK, and dropping dotnet was a very intentional choice for us.

It is possible we will add the @Test() attribute, or other more integrated QDK testing tools, to the modern QDK. However, in order to support that, we would need some sort of underlying test framework to replace Xunit. This library could be that underlying test framework. At the very least, this library is a useful utility and a step in the right direction. This test library is also written in pure Q#, which makes it easier to integrate. Check out the changes to the signed unit tests to see the specific use case this library is targeted at.

In general, with the Modern QDK, we prioritize the Python and VS Code experiences. So any further design work done on testing will prioritize the testing experience in those environments.

Base automatically changed from alex/clean-bounds to main November 14, 2024 14:33
@sezna sezna requested a review from swernli November 14, 2024 23:13
Copy link
Collaborator

@swernli swernli left a comment

Choose a reason for hiding this comment

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

The QTest library looks good, just a question about the place it's used in one of the existing library tests. I'm fine if more usage of QTest is waiting for a follow up PR.

@sezna sezna added this pull request to the merge queue Nov 15, 2024
Merged via the queue into main with commit 2edde75 Nov 15, 2024
18 checks passed
@sezna sezna deleted the alex/qtest branch November 15, 2024 00:44
/// test results instead of printing out to output.
///
/// # Input
/// Takes a list of test cases. A test case is a tuple of `(String, () -> T, 'T)`, where
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: (String, () => T, 'T)

/// This is a good alternative to `CheckAllTestCases` when you want custom output based on the results of your tests,
/// or more control over how test results are rendered.
/// # Input
/// Takes a list of test cases. A test case is a tuple of `(String, () -> T, 'T)`, where
Copy link
Contributor

@Manvi-Agrawal Manvi-Agrawal Nov 15, 2024

Choose a reason for hiding this comment

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

Nit: Similarly here. (String, () => T, 'T) and the corresponding example update


Fact(
not Functions.CheckAllTestCases(sample_tests),
"Test harness failed to return false for a failing tests."
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: for a failing test/for the failing test(s).


function MeasureSignedIntTests() : (String, Int, (Qubit[]) => (), (Qubit[]) => Int, Int)[] {
[
("0b0001 == 1", 4, (qs) => X(qs[0]), (qs) => MeasureSignedInteger(qs, 4), 1),
Copy link
Contributor

@Manvi-Agrawal Manvi-Agrawal Nov 15, 2024

Choose a reason for hiding this comment

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

@sezna , it seems these tests should not pass since the signature of QTest.Operations.TestCases was changed. Do the current library tests get checked in CI or am I missing something here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey Manvi, I'm not seeing an issue here. The function call here does indeed use the latest api.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sezna, I realized it now, that this still accepts a tuple containing 5 values. I think I got confused by 2 factors:

  • Incorrect doc string : this seems to be copy paste of the functions namespace. Could you please fix it?
  • Unnecessary braces around (Qubit[]) in method signature: Its written as (Qubit[])=> () here and in Operations.qs. This can be written as Qubit[] => ().

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.

4 participants