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

Fixed the test_add_entry_from_hek_qr test #1717

Closed
wants to merge 1 commit into from

Conversation

Punyaslok
Copy link
Member

@Cadair @derdon
The test_add_entry_from_hek_qr() function was failing because the number 1678 in the assert statement has changed to 2133. So, assert len(database) > 0 will fix it in the long run.

@Cadair
Copy link
Member

Cadair commented Mar 29, 2016

An interesting approach, @derdon opinions?

@derdon
Copy link
Member

derdon commented Mar 29, 2016

I talked with @Punyaslok about this problem and it was my idea. As @Punyasiok said, this will not cause failing tests in the future because entries in a service will be added (or removed). My only criticism is that this PR should fix it on all similar places where the number of db entries is checked.

@Cadair
Copy link
Member

Cadair commented Mar 29, 2016

Is there not a way to check the expected number of records to add to the database from the query result?

@dpshelio
Copy link
Member

I'm not too convinced of this solution.... I thought I had found the problem of this... but I don't remember why this was happening... will update if I find what it was.

@Cadair
Copy link
Member

Cadair commented Mar 31, 2016

@dpshelio what specifically don't you like about this approach?

In my opinion this test should be checking that given we must know how the records in the database are generated from the HEK results object, we should parse the results to calculate the number of records that will be added then check that the correct number have been added. @Punyaslok said on IRC that how to do this was not immediately apparent. @derdon any ideas?

@wafels
Copy link
Member

wafels commented Mar 31, 2016

The HEK is not static, in the sense that time ranges that have already been
analyzed are occasionally re-analyzed, either by human observers or by
algorithm. Hence, numbers of events can change. By default a HEK query
always returns the most recent revision number. You can ask for older
versions by adding the keyword "rev".

http://vso.stanford.edu/hekwiki/ApplicationProgrammingInterface?action=print

Maybe this would help lend some stability to the number of events returned
by the HEK.

On Thu, Mar 31, 2016 at 4:38 AM, Stuart Mumford notifications@github.com
wrote:

@dpshelio https://github.com/dpshelio what specifically don't you like
about this approach?

In my opinion this test should be checking that given we must know how the
records in the database are generated from the HEK results object, we
should parse the results to calculate the number of records that will be
added then check that the correct number have been added. @Punyaslok
https://github.com/Punyaslok said on IRC that how to do this was not
immediately apparent. @derdon https://github.com/derdon any ideas?


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#1717 (comment)

@Cadair Cadair added [BugFix] net Affects the net submodule labels Apr 11, 2016
@ayshih
Copy link
Member

ayshih commented Apr 11, 2016

As discussed in the dev meeting, this test is conflating two purposes which should be explicitly separated:

  • Testing whether a live query result is add-able. For that, the len(database) > 0 approach is reasonable.
  • Testing whether add_from_hek_query_result() adds the correct number of entries for a known query result. For that, the appropriate input should not be a live query result, but rather a synthetic query result that is part of our offline testing suite.

@Cadair
Copy link
Member

Cadair commented Oct 12, 2016

superseded by #1902

@Cadair Cadair closed this Oct 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Affects the net submodule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants