Closed Bug 1358076 Opened 7 years ago Closed 7 years ago

[PDF gtest] Implement unit test for PDFViaEMFPrintHelper

Categories

(Core :: Printing: Output, enhancement, P1)

Product:
Core
Component:
Printing: Output
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: fatseng, Assigned: fatseng)

References

(Blocks 1 open bug)

Details

User Story

./mach gtest TestEMFConversion.*

Attachments

(2 files, 2 obsolete files)

Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper
59 bytes, text/x-review-board-request
jwatt
: review+
Details
bug 1358076 - Part2. Enable gtest for PDFViaEMFPrintHelper
59 bytes, text/x-review-board-request
jwatt
: review+
Details 
      No description provided.
Assignee: nobody → fatseng
Status: NEW → ASSIGNED
Attached patch WIP gtest (obsolete) — — Splinter Review 

    
    

    
  
(In reply to Farmer Tseng[:fatseng] from comment #1)
> Created attachment 8882661 [details] [diff] [review]
> WIP gtest

Upload WIP patch.

    
  
User Story: (updated)

    
    

    
  
I wrote following gtest to verify operating PDFium.
1. WindowsEMF Init test
2. Convert PDFiumTest.pdf to an EMF file and check the file size
3. Input a PDF file which does not exist
4. Test unreasonable width or height

    
    

    
      
  


  

    
    

    
      
  

    
  

        Attachment #8885147 -
        Flags: feedback?(brsun)

    
  

        Attachment #8884224 -
        Flags: feedback?(brsun)

    
  

        Attachment #8885148 -
        Flags: feedback?(brsun)

    
  

        Attachment #8882661 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8885147 [details]
Bug 1358076 - Part1. Create an API to load a PDF file by |const char*|

https://reviewboard.mozilla.org/r/156012/#review161178

::: widget/windows/PDFViaEMFPrintHelper.h:36
(Diff revision 1)
>    PDFViaEMFPrintHelper();
>    ~PDFViaEMFPrintHelper();
>  
>    /** Loads the specified PDF file. */
>    NS_IMETHOD OpenDocument(nsIFile *aFile);
> +  NS_IMETHOD OpenDocument(const char* aFile);

This function might not be a mendatory function for using or testing PDFViaEMFPrintHelper.

    
    

    
  
Comment on attachment 8884224 [details]
Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

https://reviewboard.mozilla.org/r/155170/#review161164

I found lots of usage of ASSERT_* in other gtest files but very few usage of GTEST_ASSERT_*. I am not sure what's the difference between GTEST_ASSERT_* and ASSERT_*, but probably we should use ASSERT_* ones.

::: widget/windows/tests/gtest/TestPDFtoEMF.cpp:24
(Diff revision 2)
> +
> +UniquePtr<PDFViaEMFPrintHelper>
> +CreatePrintHelper(const char* aFile)
> +{
> +  UniquePtr<PDFViaEMFPrintHelper> PDFPrintHelper = MakeUnique<PDFViaEMFPrintHelper>();
> +  nsresult rv = PDFPrintHelper->OpenDocument(aFile);

Suggest to use nsIFile interface if possible. So that we can save one unused public function of PDFViaEMFPrintHelper.

::: widget/windows/tests/gtest/TestPDFtoEMF.cpp:39
(Diff revision 2)
> +}
> +
> +NS_IMETHODIMP
> +GetEMFFilePath(nsAutoCString& aPath)
> +{
> +  WCHAR tempPathBuffer[MAX_PATH+1];

nit: [MAX_PATH + 1] (with space before and after the + operator)

::: widget/windows/tests/gtest/TestPDFtoEMF.cpp:51
(Diff revision 2)
> +  tempLocalFile->InitWithPath(tempPath);
> +  tempLocalFile->AppendNative(NS_LITERAL_CSTRING("gtest.emf"));
> +  return tempLocalFile->GetNativePath(aPath);
> +}
> +
> +// WindowsEMF Init test

Suggest separate this test into another test file.

::: widget/windows/tests/gtest/TestPDFtoEMF.cpp:59
(Diff revision 2)
> +  WindowsEMF emf;
> +  ASSERT_TRUE(emf.InitForDrawing());
> +}
> +
> +// Convert PDFiumTest.pdf to a EMF file and check the file size
> +TEST(PDFtoEMF, SaveEMFtoFile)

Rename the test with something like TestEMFConversion.

::: widget/windows/tests/gtest/TestPDFtoEMF.cpp:62
(Diff revision 2)
> +
> +// Convert PDFiumTest.pdf to a EMF file and check the file size
> +TEST(PDFtoEMF, SaveEMFtoFile)
> +{
> +  UniquePtr<PDFViaEMFPrintHelper> PDFHelper =
> +    CreatePrintHelper("PDFiumTest.pdf");

Suppose we will have not only one PDF file for testing, so I would suggest to use a more informative name for this PDF file (ex. PDFiumTest_4961_7016.pdf or something more meaningful to the purpose).

::: widget/windows/tests/gtest/TestPDFtoEMF.cpp:63
(Diff revision 2)
> +// Convert PDFiumTest.pdf to a EMF file and check the file size
> +TEST(PDFtoEMF, SaveEMFtoFile)
> +{
> +  UniquePtr<PDFViaEMFPrintHelper> PDFHelper =
> +    CreatePrintHelper("PDFiumTest.pdf");
> +  GTEST_ASSERT_NE(PDFHelper, nullptr);

ASSERT_NE

::: widget/windows/tests/gtest/TestPDFtoEMF.cpp:66
(Diff revision 2)
> +  UniquePtr<PDFViaEMFPrintHelper> PDFHelper =
> +    CreatePrintHelper("PDFiumTest.pdf");
> +  GTEST_ASSERT_NE(PDFHelper, nullptr);
> +
> +  nsAutoCString emfPath;
> +  nsresult rv = GetEMFFilePath(emfPath);

Suggest directly get nsAutoString instead of converting from UTF8 to UTF16 each time afterwards.

::: widget/windows/tests/gtest/TestPDFtoEMF.cpp:83
(Diff revision 2)
> +  PDFHelper->CloseDocument();
> +
> +  RefPtr<nsLocalFile> savedFile = new nsLocalFile();
> +  savedFile->InitWithPath(NS_ConvertUTF8toUTF16(emfPath));
> +  RefPtr<nsFileInputStream> inputStream = new nsFileInputStream();
> +  rv = inputStream->Init(savedFile, -1, -1, 0);

nit: add more comments (i.e. /* ioFlags */, /* perm */, /* behaviorFlags */) for those hardcoded default values.

::: widget/windows/tests/gtest/TestPDFtoEMF.cpp:91
(Diff revision 2)
> +    return;
> +  }
> +
> +  int64_t size = 0;
> +  inputStream->GetSize(&size);
> +  GTEST_ASSERT_EQ(size, 19175024);

ASSERT_EQ.

By the way, would it be doable to add the corresponding EMF file into m-c for this gtest? So that we can directly compare the equality of their content.

::: widget/windows/tests/gtest/TestPDFtoEMF.cpp:97
(Diff revision 2)
> +  inputStream = nullptr;
> +  savedFile->Remove(/* aRecursive */ false);
> +}
> +
> +// Input a PDF file which does not exist
> +TEST(PDFtoEMF, InputNullPDF)

Rename the test with something like TestInputNonExistingPDF.

::: widget/windows/tests/gtest/TestPDFtoEMF.cpp:104
(Diff revision 2)
> +  UniquePtr<PDFViaEMFPrintHelper> PDFHelper = CreatePrintHelper("null.pdf");
> +  GTEST_ASSERT_EQ(PDFHelper, nullptr);
> +}
> +
> +// Test unreasonable width or height
> +TEST(PDFtoEMF, TestWidthAndHeight)

Rename the test with something like TestInsufficientWithAndHeight.

::: widget/windows/tests/gtest/TestPDFtoEMF.cpp:108
(Diff revision 2)
> +// Test unreasonable width or height
> +TEST(PDFtoEMF, TestWidthAndHeight)
> +{
> +  UniquePtr<PDFViaEMFPrintHelper> PDFHelper =
> +    CreatePrintHelper("PDFiumTest.pdf");
> +  GTEST_ASSERT_NE(PDFHelper, nullptr);

ASSERT_NE

::: widget/windows/tests/gtest/moz.build:7
(Diff revision 2)
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, you can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +Library('gfxtest')

Any reason this line is needed?

    
    

    
  
Comment on attachment 8885148 [details]
bug 1358076 - Part2. Enable gtest for PDFViaEMFPrintHelper

https://reviewboard.mozilla.org/r/156014/#review161186

::: widget/windows/moz.build:124
(Diff revision 1)
>    LOCAL_INCLUDES += [
>      '/gfx/skia/skia/include/config',
>      '/gfx/skia/skia/include/core',
>      '/modules/pdfium/pdfium/public',
>    ]
> +  TEST_DIRS += ['tests/gtest']

|gtest| probably deserves its own directory instead of living inside |tests| folder (by glancing other gtest folder structure of other modules).

    
  

        Attachment #8885147 -
        Flags: feedback?(brsun)

    
  

        Attachment #8884224 -
        Flags: feedback?(brsun)

    
  

        Attachment #8885148 -
        Flags: feedback?(brsun)
Priority: -- → P1

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8885147 -
        Attachment is obsolete: true

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8884224 -
        Flags: feedback?(brsun)

    
  

        Attachment #8885148 -
        Flags: feedback?(brsun)

    
    

    
  
Comment on attachment 8884224 [details]
Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

https://reviewboard.mozilla.org/r/155170/#review161164

> ASSERT_EQ.
> 
> By the way, would it be doable to add the corresponding EMF file into m-c for this gtest? So that we can directly compare the equality of their content.

I will create another bug to follow up.

    
  

        Attachment #8884224 -
        Flags: feedback?(brsun) → feedback+

    
  

        Attachment #8885148 -
        Flags: feedback?(brsun) → feedback+

    
  
User Story: (updated)

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
I would like dump GDI commands from EMF file and compare with reference one in gtest.

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8884224 [details]
Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

I dump GDI commands from EMF file and compare with the corresponding EMF file.

        Attachment #8884224 -
        Flags: feedback?(brsun)

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8884224 -
        Flags: feedback?(brsun)

    
    

    
  
Comment on attachment 8884224 [details]
Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

https://reviewboard.mozilla.org/r/155170/#review164012

::: widget/windows/gtest/TestEMFConversion.cpp:48
(Diff revision 8)
> +
> +  return PDFPrintHelper;
> +}
> +
> +NS_IMETHODIMP
> +GetSpecifiedFilePathfromTempFolder(nsAutoString& aPath, const char* aFile)

Suggest to use a more precise name for this function. Do you mean an absolute path or a canonical path by "SpecifiedFilePath"?

::: widget/windows/gtest/TestEMFConversion.cpp:68
(Diff revision 8)
> +  }
> +  return rv;
> +}
> +
> +int CALLBACK
> +EnhMetaFileProc(HDC ahdc,

s/ahdc/aHDC

::: widget/windows/gtest/TestEMFConversion.cpp:69
(Diff revision 8)
> +  return rv;
> +}
> +
> +int CALLBACK
> +EnhMetaFileProc(HDC ahdc,
> +                HANDLETABLE* ahandleTable,

s/ahandleTable/aHandleTable

::: widget/windows/gtest/TestEMFConversion.cpp:75
(Diff revision 8)
> +                const ENHMETARECORD* aRecord,
> +                int aObjectsCount,
> +                LPARAM aParam)
> +{
> +  // Save a GDI command to a Vector
> +  Vector<DWORD>* contents = (Vector<DWORD>*) (aParam);

Would it be possible to store and to compare all parameters within ENHMETARECORD structure?

::: widget/windows/gtest/TestEMFConversion.cpp:95
(Diff revision 8)
> +  HENHMETAFILE hemf = ::GetEnhMetaFileW(aEMFPath.get());
> +  if (hemf == NULL) {
> +    return false;
> +  }
> +
> +  HDC hdc = ::GetDC(NULL);

s/hdc/hScreenDC/ if you are meant to get the DC of the entire screen.

::: widget/windows/gtest/TestEMFConversion.cpp:101
(Diff revision 8)
> +  if (hdc == NULL) {
> +    return false;
> +  }
> +
> +  RECT rect = {0, 0, aPageWidth, aPageHeight};
> +  bool result = ::EnumEnhMetaFile(hdc, hemf, EnhMetaFileProc,

Instead of using the DC of the entire screen for testing, I would suggest to use CreateCompatibleDC and CreateCompatibleBitmap to create a memory DC for testing purpose.

::: widget/windows/gtest/TestEMFConversion.cpp:145
(Diff revision 8)
> +
> +  int64_t size = 0;
> +  inputStream->GetSize(&size);
> +  ASSERT_GT(size, 0);
> +
> +  // Enumerate GDI commands from gtest.emf and save to a buffer

Suggest to split codes with a new function for comparing two existing EMF files.

::: widget/windows/gtest/TestEMFConversion.cpp:153
(Diff revision 8)
> +                           pageWidth, pageHeight);
> +  ASSERT_TRUE(result);
> +
> +  savedFile = nullptr;
> +
> +  // Get the reference file, PrinterTestPage_ref.emf.

Suggest to use a function for getting files under the current directory to reduce the duplication of codes. Probably it's also worth to further reduce the duplication with some flexibility to get the temporary path with this new function.

::: widget/windows/gtest/TestEMFConversion.cpp:177
(Diff revision 8)
> +  Vector<DWORD> emfFileContentsRef;
> +  result = SaveGDICommands(emfPathRef, &emfFileContentsRef,
> +                           pageWidth, pageHeight);
> +  ASSERT_TRUE(result);
> +
> +  // Compare GDI commands

Add an assertion for the equality of the number of records in both vectors.

::: widget/windows/gtest/moz.build:18
(Diff revision 8)
> +    '/widget/windows',
> +]
> +
> +TEST_HARNESS_FILES.gtest += [
> +    'PrinterTestPage.pdf',
> +    'PrinterTestPage_ref.emf',

Could you help share how this EMF file is generated?

    
  

        Attachment #8884224 -
        Flags: feedback?(brsun)

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8884224 -
        Flags: feedback?(brsun)

    
  
Blocks: 1382512

    
    

    
  
Comment on attachment 8884224 [details]
Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

https://reviewboard.mozilla.org/r/155170/#review165602

::: widget/windows/gtest/TestEMFConversion.cpp:22
(Diff revision 9)
> +using namespace mozilla;
> +namespace mozilla {
> +namespace widget {
> +
> +NS_IMETHODIMP
> +GetRealPath(const char* aSpecialDirName,

|GetRealPath| doesn't reflect the real functionality of this function.

::: widget/windows/gtest/TestEMFConversion.cpp:24
(Diff revision 9)
> +namespace widget {
> +
> +NS_IMETHODIMP
> +GetRealPath(const char* aSpecialDirName,
> +            const char* aFileName,
> +            nsIFile** aResult)

nit: Suggest to use smart pointers instead of raw pointers if it is possible. How about returning nsIFile instead of passing it as an out-param?

::: widget/windows/gtest/TestEMFConversion.cpp:42
(Diff revision 9)
> +NS_IMETHODIMP
> +GetRealPath(const char* aSpecialDirName,
> +            const char* aFileName,
> +            nsAutoString& aPath)
> +{
> +  RefPtr<nsIFile> tempLocalFile;

nsCOMPtr<nsIFile> file;

ref: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/nsCOMPtr_versus_RefPtr#General_Rule_of_Thumb_for_nsCOMPtr_versus_RefPtr

::: widget/windows/gtest/TestEMFConversion.cpp:47
(Diff revision 9)
> +  RefPtr<nsIFile> tempLocalFile;
> +  nsresult rv = GetRealPath(aSpecialDirName, aFileName,
> +                            getter_AddRefs(tempLocalFile));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsAutoCString localPath;

nit: the prefix 'local' doesn't give this variable more meaning from others. Probably |path| would suffice already. The same comment applys to other lines below as well.

::: widget/windows/gtest/TestEMFConversion.cpp:81
(Diff revision 9)
> +                const ENHMETARECORD* aRecord,
> +                int aObjectsCount,
> +                LPARAM aParam)
> +{
> +  // Save a GDI command to a Vector
> +  Vector<ENHMETARECORD>* contents = (Vector<ENHMETARECORD>*) (aParam);

I guess ENHMETARECORD structure itself doesn't contain the whole records. So if you want to compare all parameters, you probably need to go beyound |dParm|.

::: widget/windows/gtest/TestEMFConversion.cpp:170
(Diff revision 9)
> +                                          pageWidth, pageHeight);
> +  ASSERT_TRUE(result);
> +
> +  PDFHelper->CloseDocument();
> +
> +  // Check the EMF file size if it is great than zero

// Check the EMF file size and make sure it is greater than zero

::: widget/windows/gtest/TestEMFConversion.cpp:184
(Diff revision 9)
> +
> +  int64_t size = 0;
> +  inputStream->GetSize(&size);
> +  ASSERT_GT(size, 0);
> +
> +  nsCOMPtr<nsIFile> emfRef;

I guess you can get a real path directly from your utility function instead of retriving from nsIFile manually here, right?

    
  

        Attachment #8884224 -
        Flags: feedback?(brsun)

    
  
Blocks: 1385779

    
    

    
  
Comment on attachment 8884224 [details]
Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

https://reviewboard.mozilla.org/r/155170/#review168242

::: widget/windows/gtest/TestEMFConversion.cpp:81
(Diff revision 9)
> +                const ENHMETARECORD* aRecord,
> +                int aObjectsCount,
> +                LPARAM aParam)
> +{
> +  // Save a GDI command to a Vector
> +  Vector<ENHMETARECORD>* contents = (Vector<ENHMETARECORD>*) (aParam);

Every Windows platform might generate a little bit different content of EMF. Comparing all parameters would be failed. I compared the GDI commands in this test to make sure the EMF is correct and created a followed up bug to compare the bitmap (Bug 1382512).

    
    

    
  
Comment on attachment 8884224 [details]
Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

https://reviewboard.mozilla.org/r/155170/#review165602

> I guess ENHMETARECORD structure itself doesn't contain the whole records. So if you want to compare all parameters, you probably need to go beyound |dParm|.

Every Windows platform might generate a little bit different content of EMF. Comparing all parameters would be failed. I compared the GDI commands in this test to make sure the EMF is correct and created a followed up bug to compare the bitmap (Bug 1382512).

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8884224 -
        Flags: feedback?(brsun)

    
    

    
  
> Every Windows platform might generate a little bit different content of EMF.

So...if this is the case, I am curious about what can be guaranteed by comparing a run-time generated EMF with a pre-generated EMF file. Can we foresee any false-positive and false-negative cases by simply comparing GDI commands?

    
    

    
  
(In reply to Bruce Sun [:brsun] from comment #36)
> > Every Windows platform might generate a little bit different content of EMF.
> 
> So...if this is the case, I am curious about what can be guaranteed by
> comparing a run-time generated EMF with a pre-generated EMF file. Can we
> foresee any false-positive and false-negative cases by simply comparing GDI
> commands?

I can't guarantee it. I will remove GDI commands comparison and implement bitmap comparison(Bug 1382512).

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8884224 -
        Flags: feedback?(brsun)

    
    

    
  
Comment on attachment 8884224 [details]
Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

https://reviewboard.mozilla.org/r/155170/#review169654

::: widget/windows/gtest/TestEMFConversion.cpp:70
(Diff revision 11)
> +
> +  return PDFPrintHelper;
> +}
> +
> +// Convert PrinterTestPage.pdf to a EMF file and compare GDI commands with
> +// reference one

Please change comments accordingly.

    
  

        Attachment #8884224 -
        Flags: feedback?(brsun) → feedback+

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8884224 [details]
Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

I implemented following test cases to verify PDFViaEMFPrintHelper.
1. Test PDF to EMF conversion.
2. Test a PDF file which does not exist.
3. Test insufficient width and height.

        Attachment #8884224 -
        Flags: review?(jwatt)

    
  

        Attachment #8885148 -
        Flags: review?(jwatt)

    
    

    
  
Comment on attachment 8884224 [details]
Bug 1358076 - Part1. Add a gtest to verify PDFViaEMFPrintHelper

https://reviewboard.mozilla.org/r/155170/#review170936

::: commit-message-7b5e7:1
(Diff revision 12)
> +Bug 1358076 - Part1. Implement gtest to verify PDFViaEMFPrintHelper

Make this "Add a gtest..."

::: commit-message-7b5e7:4
(Diff revision 12)
> +Bug 1358076 - Part1. Implement gtest to verify PDFViaEMFPrintHelper
> +
> +PrinterTestPage.pdf is from
> +http://zsory-furdo.hu/evcms_medias/upload/files/testfile.pdf

I believe this is originally from:

https://opensource.apple.com/source/cups/cups-136.9/cups/test/testfile.pdf

Please change the link.

        Attachment #8884224 -
        Flags: review?(jwatt) → review+

    
    

    
  
Comment on attachment 8885148 [details]
bug 1358076 - Part2. Enable gtest for PDFViaEMFPrintHelper

https://reviewboard.mozilla.org/r/156014/#review170940

        Attachment #8885148 -
        Flags: review?(jwatt) → review+

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Pushed by fatseng@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0e1dfba3554e
Part1. Add a gtest to verify PDFViaEMFPrintHelper r=jwatt
https://hg.mozilla.org/integration/autoland/rev/76650f11d581
Part2. Enable gtest for PDFViaEMFPrintHelper r=jwatt

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/0e1dfba3554e
https://hg.mozilla.org/mozilla-central/rev/76650f11d581
Status: ASSIGNED → RESOLVED
Closed: 7 years ago

          status-firefox57:
          --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

    
  
Blocks: 1388951

    
  
User Story: (updated)

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: