Use `.knapsack_pro` directory for temporary files instead of the `tmp` directory in the user's project directory #155

ArturT · 2021-08-06T15:27:09Z

Problem

So far knapsack_pro gem generated temporary files in the tmp/knapsack_pro directory but people sometimes have RSpec hooks to clean up thetmp folder in their project. This leads to removing files generated by knapsack_pro gem.

Solution

Use a different directory for temporary files. knapsack_pro gem will generate files in the .knapsack_pro directory in the user project folder.

.knapsack_pro folder will be ignored by git out of the box because we generate .knapsack_pro/.gitignore file that ignores all content in the .knapsack_pro folder.

… way error will be more explicit when the report is missing.

…ting might be a very narrow tests list. Most likely, you run only tests with a specified tag, and the number of test files with the tag was lower than the number of parallel CI nodes.

rspec --seed 62479

shadre

Adding some questions and comments

shadre · 2021-08-11T15:01:16Z

CHANGELOG.md

@@ -1,5 +1,13 @@
 # Change Log

+### 3.1.0
+
+* Use `.knapsack_pro` directory for temporary files instead of the `tmp` folder in the user project folder


I think this should read user's project. I would also change "folder" to "directory" (despite the duplication).

shadre · 2021-08-11T15:03:45Z

lib/knapsack_pro/adapters/base_adapter.rb

@@ -39,14 +39,14 @@ def self.verify_bind_method_called
            puts "\n\n"
            KnapsackPro.logger.error('-'*10 + ' Configuration error ' + '-'*50)
            KnapsackPro.logger.error("You forgot to call #{self}.bind method in your test runner configuration file. It is needed to record test files time execution. Please follow the installation guide to configure your project properly https://docs.knapsackpro.com/knapsack_pro-ruby/guide/")
-            KnapsackPro.logger.error("If you already have #{self}.bind method added and you still see this error then one of your tests must had to delete tmp/knapsack_pro directory from the disk accidentally. Please ensure you do not remove tmp/knapsack_pro directory: https://knapsackpro.com/faq/question/why-all-test-files-have-01s-time-execution-for-my-ci-build-in-user-dashboard")
+            KnapsackPro.logger.error("If you already have #{self}.bind method added and you still see this error then one of your tests must had to delete .knapsack_pro directory from the disk accidentally. Please ensure you do not remove .knapsack_pro directory: https://knapsackpro.com/faq/question/why-all-test-files-have-01s-time-execution-for-my-ci-build-in-user-dashboard")


Pretty sure "must had to delete" is ungrammatical. How about "must have deleted"?

Also, I think we are missing "the" in "the .knapsack_pro directory" (2 occurrences)

shadre · 2021-08-11T15:21:01Z

lib/knapsack_pro/config/temp_files.rb

+      end
+
+      def ensure_temp_directory_exists
+        unless File.exists?(gitignore_file_path)


This method is deprecated in ruby 2.7.1, and doesn't exist in ruby 3.0.2. There is an #exist? method, but I'm not sure if they are effectively the same.

I was dealing with ensuring a directory exists in my past side-project. I looked up the code and noticed I was using the Dir class instead. I don't remember offhand why I chose it over File - you probably know the difference better. With Dir you can also create the directory.

In case it's helpful to you, here is how I handled a similar case back then:

create_needed_dirs! def create_needed_dirs! create!('foo') create!('bar') end def create!(directory) return false if Dir.exist?(directory) Dir.mkdir(directory) && true end

Good catch. I've used File.exist? in the whole codebase.

shadre · 2021-08-11T15:25:51Z

lib/knapsack_pro/config/temp_files.rb

+        temp_files.temp_directory_path
+      end
+
+      def ensure_temp_directory_exists


Do we need to keep this method and the #temp_directory_path a part of the public interface of this class? From a quick glance, it seems that only the class method is being called from outside? 🤔 It's also the only one that's being (directly) tested in the spec.

When I move code to private section:

private def ensure_temp_directory_exists unless File.exists?(gitignore_file_path) create_temp_directory create_gitignore_file end end # relative to the directory where you run knapsack_pro gem (user's project) def temp_directory_path '.knapsack_pro' end

then I get error:

$ rspec ./spec/knapsack_pro/config/temp_files_spec.rb Randomized with seed 3043 KnapsackPro::Config::TempFiles .temp_directory_path .gitignore file has content (FAILED - 1) creates .gitignore file (FAILED - 2) returns temporary directory path (FAILED - 3) Failures: 1) KnapsackPro::Config::TempFiles.temp_directory_path .gitignore file has content Failure/Error: temp_files.ensure_temp_directory_exists NoMethodError: private method `ensure_temp_directory_exists' called for #<KnapsackPro::Config::TempFiles:0x00007f911c9ed6d0> # ./lib/knapsack_pro/config/temp_files.rb:8:in `temp_directory_path' # ./spec/knapsack_pro/config/temp_files_spec.rb:5:in `block (3 levels) in <top (required)>' # ./spec/knapsack_pro/config/temp_files_spec.rb:18:in `block (3 levels) in <top (required)>' # /Users/artur/.rvm/gems/ruby-3.0.2/gems/webmock-3.13.0/lib/webmock/rspec.rb:37:in `block (2 levels) in <top (required)>'

When I move code to protected I get error as well:

1) KnapsackPro::Config::TempFiles.temp_directory_path creates .gitignore file Failure/Error: temp_files.ensure_temp_directory_exists NoMethodError: protected method `ensure_temp_directory_exists' called for #<KnapsackPro::Config::TempFiles:0x00007fbb43a47c88> # ./lib/knapsack_pro/config/temp_files.rb:8:in `temp_directory_path' # ./spec/knapsack_pro/config/temp_files_spec.rb:5:in `block (3 levels) in <top (required)>' # ./spec/knapsack_pro/config/temp_files_spec.rb:13:in `block (3 levels) in <top (required)>' # /Users/artur/.rvm/gems/ruby-3.0.2/gems/webmock-3.13.0/lib/webmock/rspec.rb:37:in `block (2 levels) in <top (required)>'

We call the following methods in class method temp_directory_path so they probably must be public.

temp_files.ensure_temp_directory_exists temp_files.temp_directory_path

Yes, this is because we are trying to access the private method from outside the object.

If we are adamant about the public method remaining a class method, we could change all the other methods to be private class methods.

shadre · 2021-08-11T15:28:19Z

lib/knapsack_pro/config/temp_files.rb

+      end
+
+      def gitignore_file_content
+        "# This directory is used by knapsack_pro gem for temporary files during tests runtime.\n" <<


I think that "for storing temporary files" sounds more proper.

shadre · 2021-08-11T16:26:58Z

lib/knapsack_pro/tracker.rb

@@ -110,6 +110,7 @@ def save_prerun_tests_report(hash)
    end

    def read_prerun_tests_report
+      raise "Report #{prerun_tests_report_path} doest not exist on the disk. Most likely, it was removed accidentally. Please report the bug to the support team." unless File.exists?(prerun_tests_report_path)


Maybe we can add a link here, or at least specify that we mean the Knapsack support team?

shadre · 2021-08-11T16:28:55Z

spec/knapsack_pro/config/temp_files_spec.rb

+    end
+
+    it 'creates .gitignore file' do
+      expect(File.exists?(gitignore_file_path)).to be false


FYI there is a nice DSL for expectations like that https://relishapp.com/rspec/rspec-expectations/v/2-0/docs/matchers/expect-change

e.g. expect{Counter.increment}.to change{Counter.count}.from(0).to(1)

(just wanted to share this in case you forgot about it, the current one is cool by all means)

shadre · 2021-08-11T16:30:21Z

spec/knapsack_pro/config/temp_files_spec.rb

+      expect(File.exists?(gitignore_file_path)).to be true
+    end
+
+    it '.gitignore file has content' do


Do I understand correctly that the most important line in the file's content is the one with the asterisk (*)? If so, then maybe it'd be better to check the inclusion of it instead?

shadre · 2021-08-11T16:31:58Z

spec/knapsack_pro/slow_test_file_determiner_spec.rb

@@ -67,7 +67,7 @@

    context 'when json report does not exist' do
      it do
-        expect { subject }.to raise_error(RuntimeError, 'Report with slow test files was not generated yet. If you have enabled split by test cases https://github.com/KnapsackPro/knapsack_pro-ruby#split-test-files-by-test-cases and you see this error it means that your tests accidentally cleaned up tmp/knapsack_pro directory. Please do not remove this directory during tests runtime!')
+        expect { subject }.to raise_error(RuntimeError, 'Report with slow test files was not generated yet. If you have enabled split by test cases https://github.com/KnapsackPro/knapsack_pro-ruby#split-test-files-by-test-cases and you see this error it means that your tests accidentally cleaned up .knapsack_pro directory. Please do not remove this directory during tests runtime!')


Suggestions for the error msg:

"The report with slow test files"

I would surround the url with the parens so that it doesn't disrupt the sentence.

"the .knapsack_pro directory"

shadre · 2021-08-11T16:37:57Z

lib/knapsack_pro/config/temp_files.rb

+module KnapsackPro
+  module Config
+    class TempFiles
+      def self.temp_directory_path


Have you considered separating the ensuring the temp directory exists from returning the path? Or maybe memoizing the result or sth?

Do you think there is a way to do this check once at some point, or would it always be succeptible to errors?

The reason I'm wondering about this is that it seems that we are using this .temp_directory_path method in many places to just get the correct path, but each time we do this it would now trigger an I/O read operation. Would be great to avoid that if it's avoidable.

I was thinking about it and I decided that checking if file exists on the disk is good enough. We already do some IO operations in queue mode like saving reports and it shouldn't have significant impact on performance.

If I add memorization to class method then this causes random tests failures depending on the order of tests executed because class method memorization is preserved for each call of class method.

For instance I could do:

def self.temp_directory_path @temp_directory_path ||= begin temp_files = new temp_files.ensure_temp_directory_exists temp_files.temp_directory_path end end

and then when I run tests I get randomly different tests failures:

[14:47:44] artur@Artur-MBP: ~/Documents/github/knapsack-pro/knapsack_pro-ruby temporary-files-directory! ruby-3.0.2 $ rspec ./spec/knapsack_pro/config/temp_files_spec.rb Randomized with seed 42508 KnapsackPro::Config::TempFiles .temp_directory_path .gitignore file has content returns temporary directory path creates .gitignore file (FAILED - 1) Failures: 1) KnapsackPro::Config::TempFiles.temp_directory_path creates .gitignore file Failure/Error: expect(File.exist?(gitignore_file_path)).to be true expected true got false # ./spec/knapsack_pro/config/temp_files_spec.rb:14:in `block (3 levels) in <top (required)>' # /Users/artur/.rvm/gems/ruby-3.0.2/gems/webmock-3.13.0/lib/webmock/rspec.rb:37:in `block (2 levels) in <top (required)>' Finished in 0.02793 seconds (files took 0.53206 seconds to load) 3 examples, 1 failure Failed examples: rspec ./spec/knapsack_pro/config/temp_files_spec.rb:11 # KnapsackPro::Config::TempFiles.temp_directory_path creates .gitignore file Randomized with seed 42508 [14:47:46] artur@Artur-MBP: ~/Documents/github/knapsack-pro/knapsack_pro-ruby temporary-files-directory! ruby-3.0.2 $ rspec ./spec/knapsack_pro/config/temp_files_spec.rb Randomized with seed 56734 KnapsackPro::Config::TempFiles .temp_directory_path returns temporary directory path .gitignore file has content (FAILED - 1) creates .gitignore file (FAILED - 2) Failures: 1) KnapsackPro::Config::TempFiles.temp_directory_path .gitignore file has content Failure/Error: expect(File.read(gitignore_file_path)).to include '# This directory is used by knapsack_pro gem for storing temporary files during tests runtime' Errno::ENOENT: No such file or directory @ rb_sysopen - .knapsack_pro/.gitignore # ./spec/knapsack_pro/config/temp_files_spec.rb:19:in `read' # ./spec/knapsack_pro/config/temp_files_spec.rb:19:in `block (3 levels) in <top (required)>' # /Users/artur/.rvm/gems/ruby-3.0.2/gems/webmock-3.13.0/lib/webmock/rspec.rb:37:in `block (2 levels) in <top (required)>' 2) KnapsackPro::Config::TempFiles.temp_directory_path creates .gitignore file Failure/Error: expect(File.exist?(gitignore_file_path)).to be true expected true got false # ./spec/knapsack_pro/config/temp_files_spec.rb:14:in `block (3 levels) in <top (required)>' # /Users/artur/.rvm/gems/ruby-3.0.2/gems/webmock-3.13.0/lib/webmock/rspec.rb:37:in `block (2 levels) in <top (required)>' Finished in 0.01396 seconds (files took 0.39825 seconds to load) 3 examples, 2 failures Failed examples: rspec ./spec/knapsack_pro/config/temp_files_spec.rb:17 # KnapsackPro::Config::TempFiles.temp_directory_path .gitignore file has content rspec ./spec/knapsack_pro/config/temp_files_spec.rb:11 # KnapsackPro::Config::TempFiles.temp_directory_path creates .gitignore file Randomized with seed 56734

I could disable memorization with custom argument def self.temp_directory_path(cached: true). For the test environment I could usecache: false but I'm not sure if this is good idea to change method behaviour only to make tests pass :/

…xists! method expect(KnapsackPro::Config::TempFiles).to receive(:ensure_temp_directory_exists!)

shadre

Well done 👍

Added a few comments about minor things.

shadre · 2021-08-17T14:02:47Z

spec/knapsack_pro/config/temp_files_spec.rb

+      expect(File.read(gitignore_file_path)).to include '# This directory is used by knapsack_pro gem for storing temporary files during tests runtime'
+      expect(File.read(gitignore_file_path)).to include '# Ignore all files, and do not commit this directory into your repository.'
+      expect(File.read(gitignore_file_path)).to include '# Learn more at https://knapsackpro.com'
+      expect(File.read(gitignore_file_path)).to include '*'


Can we test it in a way that will ensure this is a whole line containing just the asterisk? Currently as I understand it, any asterisk anywhere in the file would make this expectation pass. Business-wise we need more than that.

And if we are testing for the line with the asterisk, then maybe we could improve the example description to reflect that (maybe just updating it so that it mentions "correct content" or sth like that is good enough?)

BTW, I'd be fine with removing all of the other expectations (checking for the commented out content), as I don't think they add much.

shadre · 2021-08-17T14:05:09Z

spec/knapsack_pro/slow_test_file_determiner_spec.rb

@@ -67,7 +67,7 @@

    context 'when json report does not exist' do
      it do
-        expect { subject }.to raise_error(RuntimeError, 'Report with slow test files was not generated yet. If you have enabled split by test cases https://github.com/KnapsackPro/knapsack_pro-ruby#split-test-files-by-test-cases and you see this error it means that your tests accidentally cleaned up tmp/knapsack_pro directory. Please do not remove this directory during tests runtime!')
+        expect { subject }.to raise_error(RuntimeError, 'Report with slow test files was not generated yet. If you have enabled split by test cases https://github.com/KnapsackPro/knapsack_pro-ruby#split-test-files-by-test-cases and you see this error it means that your tests accidentally cleaned up the .knapsack_pro directory. Please do not remove this directory during tests runtime!')


gramatically, this should read "has not been generated yet"

I would also say either "The report" or "A report" depending on what makes more sense.

shadre · 2021-08-17T14:06:19Z

lib/knapsack_pro/config/temp_files.rb

+
+      private
+
+      def self.create_temp_directory


I would personally add a bang to this name and the one on L31 as well.

ArturT added 14 commits August 6, 2021 13:41

Raise error when prerun tests report does not exist on the disk. This…

a709b09

… way error will be more explicit when the report is missing.

Create KnapsackPro::Config::TempFiles and add spec

4f43966

remove tmp dir only if it exists

e2b4f1d

use temp directory path in tracker

1daee2c

use new temp directory in report

0965a3d

use new temp directory in slow test file determiner

988365f

use temp directory in base adapter

65ce2c0

use new temp directory in RSpecTestExampleDetector

fc8ccb8

remove old tmp dir const

5f111a9

use new temp dir path in message

255cbad

update message

fcbdeb9

update warning message

2f350ce

clean only .knapsack_pro temp directory

98ef84a

Update spec_helper.rb

2ef9649

ArturT added the enhancement label Aug 6, 2021

ArturT mentioned this pull request Aug 6, 2021

Use .knapsack_pro directory for temporary files - update knapsack_pro version KnapsackPro/rails-app-with-knapsack_pro#36

Merged

Update CHANGELOG.md

6217728

ArturT changed the title ~~Use .knapsack_pro directory for temporary files instead of tmp folder in the user project folder~~ Use .knapsack_pro directory for temporary files instead of the tmp folder in the user project folder Aug 6, 2021

ArturT added 11 commits August 6, 2021 17:38

use .knapsack_pro gem relative path

97af123

Update report_spec.rb

8aeb0d1

Update base_adapter_spec.rb

2864bc1

updater warning message. Remove outdate message.

3997b94

When you use knapsack_pro Regular Mode, the reason for no tests execu…

46c65be

…ting might be a very narrow tests list. Most likely, you run only tests with a specified tag, and the number of test files with the tag was lower than the number of parallel CI nodes.

remove new line

4a060b0

Fix randomly failing spec

2449713

rspec --seed 62479

mock temp_directory_path method

28b1c62

Update base_adapter_spec.rb

1636994

simplify spec

eb9ab70

extract KNAPSACK_PRO_TMP_DIR

5b993f0

ArturT requested a review from shadre August 9, 2021 19:30

shadre reviewed Aug 11, 2021

View reviewed changes

ArturT changed the title ~~Use .knapsack_pro directory for temporary files instead of the tmp folder in the user project folder~~ Use .knapsack_pro directory for temporary files instead of the tmp directory in the user's project directory Aug 13, 2021

ArturT added 16 commits August 13, 2021 11:48

Update CHANGELOG.md

4ecff24

typo: update text

880f263

add storing word

427abda

Use File.exist? instead of deprecated File.exists?

a7073c2

update message

56df05c

use more general name .knapsack_pro and fix some typos

5f12f1b

text: you've added

c81ca2e

add potential word

390a1d3

add the

f69065d

add link to support team

6a5036d

verify asterisk is present in gitignore file

5d6e5b9

explicitly call ensure_temp_directory_exists! when needed

e21f0a3

Update report_spec.rb

7756183

Update base_adapter_spec.rb

b7c3b15

Update rspec_test_example_detector_spec.rb

8b8cf7b

Update rspec_test_example_detector_spec.rb

c96ab7d

ArturT requested a review from shadre August 17, 2021 11:16

ArturT added 2 commits August 17, 2021 13:21

ignore .knapsack_pro directory so we can mock ensure_temp_directory_e…

217639f

…xists! method expect(KnapsackPro::Config::TempFiles).to receive(:ensure_temp_directory_exists!)

Use ruby 3.0.2 on CI

2afdc5c

shadre approved these changes Aug 17, 2021

View reviewed changes

ArturT added 5 commits August 17, 2021 16:28

add ! to method names

0a811c0

ensure line with asterisk

eca0920

update text

a58724b

use expect to change

5b38f78

Update temp_files_spec.rb

16dc997

ArturT merged commit 4c926b7 into master Aug 17, 2021

ArturT deleted the temporary-files-directory branch August 17, 2021 15:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `.knapsack_pro` directory for temporary files instead of the `tmp` directory in the user's project directory #155

Use `.knapsack_pro` directory for temporary files instead of the `tmp` directory in the user's project directory #155

ArturT commented Aug 6, 2021

shadre left a comment

shadre Aug 11, 2021

shadre Aug 11, 2021

shadre Aug 11, 2021 •

edited

Loading

ArturT Aug 13, 2021

shadre Aug 11, 2021

ArturT Aug 13, 2021

shadre Aug 13, 2021

shadre Aug 11, 2021

shadre Aug 11, 2021

shadre Aug 11, 2021

shadre Aug 11, 2021

shadre Aug 11, 2021

shadre Aug 11, 2021

ArturT Aug 13, 2021

shadre left a comment

shadre Aug 17, 2021

shadre Aug 17, 2021

shadre Aug 17, 2021

Use .knapsack_pro directory for temporary files instead of the tmp directory in the user's project directory #155

Use .knapsack_pro directory for temporary files instead of the tmp directory in the user's project directory #155

Conversation

ArturT commented Aug 6, 2021

Problem

Solution

shadre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shadre Aug 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shadre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Use `.knapsack_pro` directory for temporary files instead of the `tmp` directory in the user's project directory #155

Use `.knapsack_pro` directory for temporary files instead of the `tmp` directory in the user's project directory #155

shadre Aug 11, 2021 •

edited

Loading