samking / pfif-tools Goto Github PK

The test conformance document does not require an API url that will let a user 
retrieve all notes since the user can emulate that functionality by retrieving 
all notes since a given date.

However, for symmetry with the ability to test retrieval of all persons and 
because the functionality is already implemented, we could provide users with 
the ability to run this test (even though it is not required for conformance).

Now, PFIF Diff and the PFIF Validator cannot support PFIF embedded in an ATOM 
feed because the objectification code and the validation code directly deals 
with the XML structure (via utils.PfifXmlTree).  A structure similar to 
personfinder_pfif's SAX XML parser could work nicely.

Currently, if someone wants to get a copy of the test data set (so that they 
can look at it rather than having it directly added to their repository), they 
must use the command line tool.  Instead, they should be able to go to a 
website and have the data set generated.

There may be bugs that haven't yet been uncovered by unit tests or sample 
queries

If an API is broken and keeps returning the same records, it will take 
MAX_API_CALLS loops to realize it.  We could fail faster by checking after each 
API call if there were any duplicate records returned.

1. Repositories must store entry_date.                                       
2. When a repository exports PFIF, it must export entry_date.                
3. PFIF intended for import into a repository doesn't need to contain 
entry_date (the receiving repository ignores entry_date).                 

#3 is the reason why the spec doesn't say it's mandatory, but we'd rather not 
mislead on the first two requirements.  As a result, it could be useful to add 
a warning in the validation check for mandatory fields if there is not an 
entry_date.

client_repo_interoperability.ClientTester.api_write_records does no error 
checking.  utils.post_xml_to_url will return a response, but that is currently 
ignored.  We could analyze it to verify that it is an HTTP 200.

Right now, when the test data set is posted to a remote repository, it is done 
in arbitrary order because notes is a map.  Instead, it must be posted in order 
so that the entry dates will be correctly ordered.

Line numbers and lines exist in the validator.  They should exist in the diff 
tool also.

Currently, there is purely static HTML and there are purely scripted pages.  
Transitioning the purely scripted pages to Django templates could make 
everything more maintainable.

revision samking-unicode adds unicode support tests and unicode support to the 
validator, but it is still pending review ([email protected]).  

The review is held up because of a discussion about whether or not to add an 
option to turn off warnings on empty optional fields (but not on empty 
mandatory fields).  This is slightly  complicated because the PFIF spec says 
that expired records should have all fields, including mandatory fields, empty 
or nonexistent except for placeholder fields.  Also, it might make sense to 
classify blank fields as errors rather than warnings because, for instance, the 
spec says that a date should be a string in the form yyyy-mm-ddThh:mm:ssZ, and 
the empty string is not a string in that form.

An elegant solution might be to add IDs to all error messages and allow the 
user to disable messages based on ID.  

App Engine has a limit on the size of requests and responses.  This causes some 
issues:
1.  if someone tries to upload a really big file, we can't support it
2.  if someone passes in the URL of a really big file, we can't read it 
(because URL reading is currently done in one request/response rather than 
incrementally)
3.  if someone's PFIF has a lot of problems (ie, thousands of errors), we can't 
display them all.

#3 is sort of fixed by truncating output, but that truncation should be user 
configurable.
#2 is probably the easiest (and most important) to work on
#1 is probably hard to fix

utils.MessagesOutput.get_field_from_messages directly uses __dict__.  Is there 
a better way to dynamically access a field from an object?

1.  Review make_test_data and merge it in

2.  Review client_repo_interoperability_tests and merge it in.  This depends on 
make_test_data, so it must be merged in after.

client_repo_interoperability.ClientTester.compile_all_responses loops until the 
remote repository stops providing input.  A malformed URL or a faulty API can 
cause this to loop infinitely.  There needs to be either
-a max number of iterations allowed, or
-a timeout on the test as a whole

Right now, a record id in the appspot website might look like:

<span class="message_person_record_id">The relevant person_record_id is: 
example.org/person. </span>

and the same is true of other tags.  Instead, there should be another span for 
example.org/person so that, for instance, we can output record_ids as 
underlined and a user can easily tell apart the boilerplate from the data.

If the remote repo is slow, we can get a DeadlineExceededError on our end.  We 
should let them know to make their repo faster or have a faster query if this 
error happens.

Now, when I fail one of the tests, the output gives the grouped diff output.  
This is concise, but it isn't as useful as it would be if people could look at 
the response and the desired response and see the actual difference.  
Currently, they would have to manually query their own database for the 
response and look at the output from make_test_data to see the desired response.

Client-Repo Interoperability tests needs to sort the data somewhere.  
Currently, there is no sorting, and somewhere, the order of the data gets 
mangled.  As a result, when there are issues with a large number of records, 
the diff output is not in any meaningful order.

If the ability to sort is added, the user should be able to turn it off since 
there can be a lot of records, and sorting could be slow.

The pfif_validator has a simple regular expression to check for email and for 
domain name validation, but this is not rigorous or complete.  We should switch 
to using a library for these tasks.

Currently, the only way to avoid running a test is to not provide a URL for it. 
 This means that if, for instance, I want to run the changed test but have 
already passed the min_date test, I will have to wait extra time while the 
min_date test runs.  Instead, there should be a check box for each test.

Currently, there is a message summary for the validator and diff tool, but 
there is no summary for the interoperability tests.

When pfif_diff.objectify_parents encounters a record without a record_id, now 
it just prints out a message.  Is there a better way to respond to this issue?

https://docs.google.com/a/google.com/document/d/1HoCtiKjmp6j2d4d2U9QBJtehuVoANcW
c6ggxthacPRY/edit?hl=en_US&authkey=CMaK1qsI

Users don't want to have to add the data every time they run the tests since 
that's a time consuming process.  However, they shouldn't have to manually add 
data to their repo either.