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 conformance tests #1

Closed
mbrubeck opened this issue Jun 10, 2015 · 10 comments
Closed

Add conformance tests #1

mbrubeck opened this issue Jun 10, 2015 · 10 comments

Comments

@mbrubeck
Copy link
Contributor

The Unicode consortium has a suite of conformance tests for the bidirectional algorithm, which can be found at:

We should add code to tools/generate.py to download these files and generate Rust unit tests for each of the test cases here.

@vicky-katara
Copy link
Contributor

@mbrubeck , @eefriedman, I am working on generating Rust unit test cases from BidiCharacterTest.txt, and the test cases essentially use the Fields 0(input string) and 4(output indices converted to output strings) and form unit test cases of the form "assert_eq!(reorder('input'), 'output');". Could you confirm whether this is correct? I am ignoring the levels information in the test cases since those should essentially be taken care of by the implementation.

Thanks!

@mbrubeck
Copy link
Contributor Author

Yes, this sounds good. Checking the levels may be useful too in some
cases, but just checking the output is the most improtant part.

On Fri, Nov 27, 2015 at 2:19 PM, Vicky Katara notifications@github.com
wrote:

@mbrubeck https://github.com/mbrubeck , I am working on generating Rust
unit test cases from BidiCharacterTest.txt, and the test cases essentially
use the Fields 0(input string) and 4(output indices converted to output
strings) and form unit test cases of the form "assert_eq!(reorder('input'),
'output');". Could you confirm whether this is correct? I am ignoring the
levels information in the test cases since those should essentially be
taken care of by the implementation.

Thanks!


Reply to this email directly or view it on GitHub
#1 (comment).

@vicky-katara
Copy link
Contributor

@mbrubeck, thanks for the confirmation. I had a followup question to the main issue. Since the files BidiTest.txt and BidiCharacterTest.txt are live, we will have to write python code which parses the files for test cases each time, and deletes the automated test cases added previously to lib.rs, and lastly add the new ones. Is this correct?

@mbrubeck
Copy link
Contributor Author

We should re-parse and re-generate the test cases only if someone manually runs the script to do it. See this discussion for more details.

@vicky-katara
Copy link
Contributor

Got it!

Thanks a lot!

@vicky-katara
Copy link
Contributor

@mbrubeck, should we add the python code for insertion of the test cases into a new python file, or should we add code to generate.py?

@mbrubeck
Copy link
Contributor Author

mbrubeck commented Dec 1, 2015

Either way is okay with me. If it's a lot of code (more than a handful of short functions), a separate file would probably be more readable. generate.py could import the other file.

@vicky-katara
Copy link
Contributor

Combined with the code for deletion of existing test cases, it comes out to about 150 lines of code...

We'll move it to another file. Thanks @mbrubeck

@samruds1
Copy link

samruds1 commented Dec 3, 2015

@mbrubeck - Is it possible for us to have a Hangouts session or for us to call you? We have some doubts regarding the steps we have to implement as well as this step.

@mbrubeck
Copy link
Contributor Author

mbrubeck commented Dec 3, 2015

@samrudhisharma Sure, if you're available sometime today. You can find me in #servo on irc.mozilla.org to arrange a time, or just try calling +Matt Brubeck on Google Hangouts.

bors-servo pushed a commit that referenced this issue May 15, 2017
Add Conformance Test and implement L1 rule

Initial results: `60494 test cases failed! (196253 passed)`

Because of the current limitations of rust's test framework, and the huge number of failures, the base (not including matching pairs) conformance test is executed in one run, and a summary is panic'ed (if there's any failure) and for the integration test to pass, it's marked as `should_panic`, with summary of the test run as `expected` string.

Fix #1

To be able to implement L1, we need access to more information from
`BidiInfo`, namely `original_classes` of the `text`, in `visual_runs()`,
which would mean it should pass through `reorder_line()`.

The fact that information from `BidiInfo` is needed for both steps of
the public API (generating `BidiInfo` and consuming it
per-paragraph/per-level) made me change the API design and move these
methods into `impl BidiInfo`.

Then, since we needed access to `text` for every `BidiInfo` consumption,
I added a reference to `text` to `BidiInfo`, which also enables more
compile-time checks for `BidiInfo` isntance not outliving the text in
the user code.

NOTE: We are already breaking API in version 0.3.0 and paving for full
spec support is a good reason to do so, IMHO.

The L1 rule works by one pass on the text of the line.

Conformance Test: this implementation reduces the number of failures
from 60494 to 23770 (out of total 256747 cases).

Fix #2

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/unicode-bidi/30)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants