Skip to content
This repository has been archived by the owner on Nov 3, 2022. It is now read-only.

Wait for travel advice to be published before asserting #59

Merged
merged 1 commit into from
Nov 10, 2017

Conversation

RyanMacG
Copy link
Contributor

@RyanMacG RyanMacG commented Nov 8, 2017

We found that the upload attachments spec would sometimes fail to find
the text it was expecting even after the url returned a 200

@cbaines
Copy link
Contributor

cbaines commented Nov 8, 2017

I'm a bit unsure about this, as I am struggling to think of a scenario where the frontend would return a response with an ok status, but not an ok body...?

Take the draft preview for example, I would expect that it would 404, then be there, and the status would be 200. If you are seeing this happening, would it be possible to display the body, so that what was actually returned can be looked at in more detail?

@adrianclay
Copy link
Contributor

Thinking about it I agree, it does seem odd.

I changed 200 check to make a GET and also print out the body, when I got a flicker I dumped that into the attached file.

Thanks for your help.

congo-incorrect.html

congo-correct.html

Diff of the two:

48c48
< <meta name="csrf-token" content="Y4BM7WVfl/hzMtseH9V7te0N0Uiac4nsEqZMrNP5lwjcTV7XUT8FTBNgxW2Tn6FUoRK4uy6tBatdoYRZgv5ipA==">
---
> <meta name="csrf-token" content="gDH5v/wTAKaZGcdGEQPhUACHSvPpqPvcu0pn7QumsfVU9GIO+1znQ9g2mx4FO0QunAxWP5gzzVw+tpxRHXMBtQ==">
52a53
> <meta name="description" content="">
133a135,137
>         <li>
>           <a href="/foreign-travel-advice/congo/a-time-to-kill">A Time to Kill</a>
>         </li>
167a172,176
>           <dt>Latest update:</dt>
>           <dd>
>               <p>Dolores perferendis et pariatur ipsam non consequuntur accusamus.</p>
> 
>           </dd>
172a182,196
>   <figure class="app-c-figure">
>   <img class="app-c-figure__image" src="http://assets-origin.dev.gov.uk/media/5a03413fd6cbfe000b5209f2/congo_map.jpg" alt="">
>   <figcaption class="app-c-figure__figcaption">        
> <a class="app-c-download-link" href="http://assets-origin.dev.gov.uk/media/5a03413fd6cbfe000b5209f3/congo_travel_advice.pdf">
>   <svg class="app-c-download-link__icon" aria-hidden="true" viewbox="0 0 25 25" height="25" width="25" xmlns="http://www.w3.org/2000/svg">
>     <g>
>       <path d="M23 5h2v20H5v-2h18V5z"></path>
>       <path d="M0 0h20v20H0z"></path>
>     </g>
>   </svg>Download map (PDF)
> </a>
> 
> </figcaption>
> </figure>
> 
178c202
<   
---
>   <p>Minus consequuntur suscipit velit. Labore exercitationem porro ea quia. Rerum numquam molestiae repellendus nihil temporibus. 1510162750</p>
183c207,223
<     
---
>     <nav class="pub-c-pagination" role="navigation" aria-label="Pagination">
>   <ul class="pub-c-pagination__list">
>       <li class="pub-c-pagination__item pub-c-pagination__item--next">
>         <a href="/foreign-travel-advice/congo/a-time-to-kill" class="pub-c-pagination__link" rel="next">
>           <span class="pub-c-pagination__link-title">
>             Next
>             <svg class="pub-c-pagination__link-icon" xmlns="http://www.w3.org/2000/svg" height="13" width="17" viewbox="0 0 17 13">
>               <path fill="currentColor" d="m10.107-0.0078125-1.4136 1.414 4.2926 4.293h-12.986v2h12.896l-4.1855 3.9766 1.377 1.4492 6.7441-6.4062-6.7246-6.7266z"></path>
>             </svg>
>           </span>
>             <span class="visually-hidden">:</span>
>             <span class="pub-c-pagination__link-label">A Time to Kill</span>
>         </a>
>       </li>
>   </ul>
> </nav>
> 
216c256
<         <input id="url" name="url" type="hidden" value="http://draft-government-frontend.dev.gov.uk/foreign-travel-advice/congo?cache=1510161907">
---
>         <input id="url" name="url" type="hidden" value="http://draft-government-frontend.dev.gov.uk/foreign-travel-advice/congo?cache=1510162752">

@cbaines
Copy link
Contributor

cbaines commented Nov 9, 2017

Thinking about it I agree, it does seem odd.

I changed 200 check to make a GET and also print out the body, when I got a flicker I dumped that into the attached file.

Given this, I think it's preferable to at least leave the test failing, rather than covering up this odd behaviour.

I'm not quite sure what could be causing this... from the URLs in the html you posted, it looks like these requests might not be going through the router, which is a little odd. I'm not sure how the docker-compose setup runs the end to end tests, but directly connecting to frontend apps isn't something that happens in the GOV.UK production environment, so ideally it wouldn't happen in the tests either.

@kevindew
Copy link
Member

kevindew commented Nov 9, 2017

Does this PR explain some of the oddities: alphagov/travel-advice-publisher#211 this might explain why we're seeing some initial empty summaries - is this in play in this scenario?

When publishing the initial travel advice for a country, the travel
advice publisher first sends over a version with a blank summary before
sending a version with the actual content. Related to 03e929d9

Occasionally the tests would pick up that blank summary document and
assert against that instead of the subsequent document.

Waiting for the content to be published remedies this race condition.
@RyanMacG RyanMacG force-pushed the wait-for-publishing branch from 556892b to ae7954e Compare November 9, 2017 16:36
Copy link
Member

@kevindew kevindew left a comment

Choose a reason for hiding this comment

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

Thanks for the commit description

@cbaines
Copy link
Contributor

cbaines commented Nov 10, 2017

kevindew approved these changes 15 hours ago

Fair enough, but permitting this intermediate broken response does worry me, I've created an issue to keep track of this #63

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants