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

Iss297 R107 discard duplicate sequence pair #336

Merged

Conversation

EyalLavi
Copy link
Collaborator

@EyalLavi EyalLavi commented Feb 6, 2017

Scenarios for R107 and 108

@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.345% when pulling 4f2de72 on iss297-R107-discard-duplicate-sequence-pair into 11a842d on kozmaz87/task/258.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.345% when pulling fe93285 on iss297-R107-discard-duplicate-sequence-pair into 11a842d on kozmaz87/task/258.

Copy link
Collaborator

@nigelmegitt nigelmegitt left a comment

Choose a reason for hiding this comment

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

The BDDs as they stand don't read well to me - it isn't clear exactly what behaviour they are testing for, so I've suggested some re-work. Hope my suggestions make sense.

Given an xml file <xml_file>
And it has sequence identifier <seq_id>
And it has sequence number <seq_n>
When a previous document exists with sequence identifier <prev_seq_id> and sequence number <prev_seq_n>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it makes more sense here to say that when the document is made available and then another document with the same id and number becomes available then when trying to add the second document it is not added. This BDD case never creates the first document, so all the work has to be done in "When a previous document exists..." which presumably creates the first document and adds it?

And it has sequence number <seq_n>
When a previous document exists with sequence identifier <prev_seq_id> and sequence number <prev_seq_n>
And the previous document has availability time <prev_avail>
Then the previous document has availability time <post_avail>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would refactor this one too - given a file with sequence id and number and it is added to the sequence with availability time <prev_avail> and an attempt is made to add a new file with same sequence id and number to the sequence then the availability time of the file in the sequence with that sequence id and number is <prev_avail>.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.345% when pulling 8aabc60 on iss297-R107-discard-duplicate-sequence-pair into 11a842d on kozmaz87/task/258.

Copy link
Collaborator

@nigelmegitt nigelmegitt left a comment

Choose a reason for hiding this comment

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

Much happier with the structure of these - thanks. Not sure about the details on the last one though, some questions there.

When a document arrives
And it has sequence identifier <seq_id_2>
And it has sequence number <seq_n_2>
Then the previous document has availability time <avail_time_2>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Surely avail_time_1 and avail_time_2 are always the same in this case? What is the case where they would differ?

When a document arrives
And it has sequence identifier <seq_id_2>
And it has sequence number <seq_n_2>
Then the previous document has availability time <avail_time_2>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just wondering: "previous" -> "first" for more clarity ?



| seq_id_1 | seq_n_1 | avail_time_1 | seq_id_2 | seq_n_2 | avail_time_2 |
| n | 1 | n | 1 | 00:00:00.000 | 00:00:00.000 |
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is n for an availability time?

Copy link
Collaborator

@nigelmegitt nigelmegitt left a comment

Choose a reason for hiding this comment

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

That makes more sense, thanks.

Obviously they need to be coded to make any sense, which would be one for @kozmaz87 to have a look at. Before going ahead, I'd like a time estimate please.

@nigelmegitt nigelmegitt added this to the Release 3 milestone Feb 6, 2017
@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.345% when pulling aa8c32c on iss297-R107-discard-duplicate-sequence-pair into 11a842d on kozmaz87/task/258.

Copy link
Collaborator

@nigelmegitt nigelmegitt left a comment

Choose a reason for hiding this comment

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

The BDDs look good, merging so we can handle the wiring later.

@nigelmegitt nigelmegitt merged commit 86ec49a into kozmaz87/task/258 Feb 15, 2017
@nigelmegitt nigelmegitt deleted the iss297-R107-discard-duplicate-sequence-pair branch February 15, 2017 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants