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.

Sign up for GitHub

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

Jump to bottom

test: add data #12

Merged
merged 18 commits into from
Jun 24, 2024
Merged

test: add data #12

merged 18 commits into from
Jun 24, 2024

Conversation

supermaxiste
Copy link
Member

Addresses #3 and #6.

It would be great if you could run a cargo test on your end to see if everything works.
Any code recommendations are more than welcome.

Major

  • Added unit tests for both io and pass_second
  • Added test data for future tool testing: index, config and ntriples file. I tried to make sure that test.nt includes elements that either do need encryption. Specifically:
<http://example.org/Bank> <http://schema.org/name> "Bank" .

Has a name predicate but should not be encrypted since Bank is not a Person.

Minor

  • Elements from the Triple struct are now public in order for the tests to access them
  • Currently all tests have test files or simple triples as input but the parser is still not implemented, so they might need to be refined after the parser is ready

@supermaxiste supermaxiste added the enhancement New feature or request label Jun 18, 2024
@supermaxiste supermaxiste requested review from gabyx and cmdoret June 18, 2024 11:27
@supermaxiste supermaxiste self-assigned this Jun 18, 2024
cmdoret
cmdoret approved these changes Jun 18, 2024
View reviewed changes
Copy link
Member

@cmdoret cmdoret left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice! cargo test works for me and it looks good I have one nitpick, but feel free to ignore.

nitpick: I would avoid complexity qualifiers ("simple") in test names and only name the function after what the test does. (e.g.: simple_encryption -> encrypt_nt_file)

@cmdoret cmdoret changed the title Feat/test data test: add data Jun 18, 2024
This was linked to issues Jun 18, 2024
Include dummy test files #6
Closed
Include tests for io.rs and encrypt.rs #3
Closed
gabyx
gabyx approved these changes Jun 18, 2024
View reviewed changes
Copy link
Contributor

@gabyx gabyx left a comment

Choose a reason for hiding this comment

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

Great stuff. Not much to add, there will come more for sure.

You are doing quite well, seriously. Starting from zero! Well done.
Some nitpicks for later.
You can directly merge this and optionally correct the nitpicks on a new PR if you want.

src/io.rs Outdated
let input: &[u8] = "<http://example.org/resource2> <http://example.org/relatedTo> <http://example.org/resource3> .\n".as_bytes();
let buffer_input: Box<dyn BufRead> = Box::new(BufReader::new(input));
parse_ntriples(buffer_input).for_each(|t| {
assert_eq!(t.subject, "A"); // to replace with http://example.org/resource2
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good! but shouldnt it be http://example.com... in the assert? You wanted it to fail for me to test?

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently the parse.nt function is turning any triple into "A", "B" and "C" ;)
That's why the test fails when using what's commented right now
https://github.com/sdsc-ordes/rdf-protect/blob/b00e799ed997d335739226e73430403a89d9d0ef/src/model.rs#L58-L62

src/pass_second.rs Show resolved Hide resolved
tests/data/config.yaml Outdated Show resolved Hide resolved
tests/data/config.yaml Outdated Show resolved Hide resolved
tests/data/config.yaml Outdated Show resolved Hide resolved
tests/data/config.yaml Outdated Show resolved Hide resolved
tests/data/config.yaml Outdated Show resolved Hide resolved
tests/data/test.nt Show resolved Hide resolved
src/pass_second.rs Outdated
// Test the parsing of a triple.
fn simple_encryption() {
let input_path = Path::new("tests/data/test.nt");
let output_path = Path::new("tests/data/");
Copy link
Contributor

Choose a reason for hiding this comment

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

the output path should be a file or - to specify stdout, should be documented probably on the function, sorry didnt wrote that yet.

supermaxiste and others added 15 commits June 20, 2024 13:17
@supermaxiste @gabyx
Update tests/data/config.yaml
b79b857 
Co-authored-by: Gabriel Nützi <gnuetzi@gmail.com>
@supermaxiste @gabyx
Update tests/data/config.yaml
11956e5 
Co-authored-by: Gabriel Nützi <gnuetzi@gmail.com>
@supermaxiste @gabyx
Update tests/data/config.yaml
0b03724 
Co-authored-by: Gabriel Nützi <gnuetzi@gmail.com>
@supermaxiste @gabyx
Update tests/data/config.yaml
4cbf11d 
Co-authored-by: Gabriel Nützi <gnuetzi@gmail.com>
@supermaxiste @gabyx
Update tests/data/config.yaml
005290c 
Co-authored-by: Gabriel Nützi <gnuetzi@gmail.com>
@supermaxiste
fix: output test file and unit test name
595a7a9
@cmdoret @supermaxiste
refactor: rio parsing (#13)
ddaec3a
@gabyx @cmdoret @supermaxiste
feat: Improve the documentation (#15)
e6d7a4e 
Co-authored-by: Cyril Matthey-Doret <cyril.mattheydoret@gmail.com>
@gabyx @supermaxiste
feat: Add a PR template (#14)
bcbaac9
@gabyx @supermaxiste
docs: Improve docs for direnv
252ee3c
@gabyx @supermaxiste
refactor: stack-allocated trait object for buffers (#16)
0ceaf09 
Signed-off-by: Gabriel Nützi <gnuetzi@gmail.com>
@supermaxiste
feat: added test for pass_second and renamed index
876bce0
@supermaxiste
refactor(tests): updated tests to new parser
61f72ea
@supermaxiste
Merge branch 'main' into feat/test-data
c7652cc
@supermaxiste
fix: formatting
2a2f4ca
@supermaxiste supermaxiste merged commit 16ebe3a into main Jun 24, 2024
@supermaxiste supermaxiste deleted the feat/test-data branch June 24, 2024 07:16
supermaxiste added a commit that referenced this pull request Jun 24, 2024
@supermaxiste @gabyx @cmdoret
test: add unit tests and data (#12)
17461ae 
Co-authored-by: Gabriel Nützi <gnuetzi@gmail.com>
Co-authored-by: Cyril Matthey-Doret <cyril.mattheydoret@gmail.com>
Co-authored-by: Gabriel Nützi <gabriel.nuetzi@sdsc.ethz.ch>
supermaxiste added a commit that referenced this pull request Jun 24, 2024
@supermaxiste @gabyx @cmdoret
test: add unit tests and data (#12)
61079cc 
Co-authored-by: Gabriel Nützi <gnuetzi@gmail.com>
Co-authored-by: Cyril Matthey-Doret <cyril.mattheydoret@gmail.com>
Co-authored-by: Gabriel Nützi <gabriel.nuetzi@sdsc.ethz.ch>
supermaxiste added a commit that referenced this pull request Jun 24, 2024
@supermaxiste @gabyx @cmdoret
test: add unit tests and data (#12)
a317d42 
Co-authored-by: Gabriel Nützi <gnuetzi@gmail.com>
Co-authored-by: Cyril Matthey-Doret <cyril.mattheydoret@gmail.com>
Co-authored-by: Gabriel Nützi <gabriel.nuetzi@sdsc.ethz.ch>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmdoret cmdoret cmdoret approved these changes

@gabyx gabyx gabyx approved these changes

Assignees

@supermaxiste supermaxiste

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Include dummy test files Include tests for io.rs and encrypt.rs
3 participants
@supermaxiste @gabyx @cmdoret