astahlman / ob-async

Asynchronous src_block execution for org-babel
343 stars 32 forks source link

fix ob-async does not work with `:results file link` based types. #52

Closed stardiviner closed 5 years ago

astahlman commented 5 years ago

🎉 Thanks @stardiviner! I'll take a look at the PR this weekend and merge if it looks OK

stardiviner commented 5 years ago

I write a test:

modified   test/ob-async-test.el
@@ -211,6 +211,26 @@ when content has been added below the source block"
                (let ((foo-contents (progn (find-file "/tmp/foo") (buffer-substring-no-properties (point-min) (point-max)))))
                  (should (string= "Don't wait on me\n" foo-contents))))))))

+(ert-deftest test-async-execute-file-block ()
+  "Test that we can insert results when header-arg :file is present"
+  (let ((buffer-contents "Here's a sh source block:
+
+  #+BEGIN_SRC sh :async :results link :file \"/tmp/foo\"
+     echo \"Don't wait on me\" > /tmp/foo
+  #+END_SRC"))
+    (with-buffer-contents
+     buffer-contents
+     (org-babel-next-src-block)
+     (ctrl-c-ctrl-c-with-callbacks
+      :pre (should (placeholder-p (results-block-contents)))
+      :post (progn
+              (should (string= "[[file:/tmp/foo]]" (results-block-contents)))
+              (let ((foo-contents (progn
+                                    (find-file "/tmp/foo")
+                                    (buffer-substring-no-properties
+                                     (point-min) (point-max)))))
+                (should (string= "Don't wait on me\n" foo-contents))))))))
+
 (ert-deftest test-async-execute-table-output ()
   "Test that we can insert table output"
   (let ((buffer-contents "Here's a source block:

But run make test get all tests failed:

Ran 18 tests in 325.615 seconds
1 expected failures
15 unexpected results:
   FAILED  test-async-ctrl-c-ctrl-c-hook
   FAILED  test-async-execute-call
   FAILED  test-async-execute-existing-sh-block
   FAILED  test-async-execute-file-block
   FAILED  test-async-execute-fresh-sh-block
   FAILED  test-async-execute-named-block
   FAILED  test-async-execute-named-block-with-results
   FAILED  test-async-execute-named-call-block
   FAILED  test-async-execute-python-block
   FAILED  test-async-execute-table-output
   FAILED  test-async-execute-tramp-block
   FAILED  test-async-return-to-point-above-block
   FAILED  test-async-return-to-point-below-block
   FAILED  test-output-to-file-with-dir
   FAILED  test-pre-execute-hook
make: *** [Makefile:19: test] Error 1

This is weird.

stardiviner commented 5 years ago

Here is part of backtrace: https://gist.github.com/d5da758eca42a55df084fea5824cd73b

astahlman commented 5 years ago

@stardiviner I think your PR should be passing tests now, but since you force-pushed Travis seems to be in a weird state and isn't running against your latest commit. Not sure what's going on there - maybe try opening a new Pull Request?

stardiviner commented 5 years ago

Seems force push can't re-trigger Travis CI. Is that need to re-open a new PR? Ok, I will.

stardiviner commented 5 years ago

Ok, I tried to create a new PR with same you and mine master branch comparing. It does not have the new PR button. Only have view this PR button. So ....

stardiviner commented 5 years ago

I checked out Travis CI log, two error reports are failed on make install-dev command.

The command "make install-dev" failed and exited with 2 during .
astahlman commented 5 years ago

That last error looks like a transient error fetching from elpa. I just re-ran the tests, and this time the error looks real:

Test test-async-execute-file-block condition:
    (ert-test-failed
     ((should
       (string= "[[file:/tmp/foo]]"
        (results-block-contents)))
      :form
      (string= "[[file:/tmp/foo]]" "/tmp/foo")
      :value nil))

I think you can fix it by changing "[[file:/tmp/foo]]" to "/tmp/foo". No need to rebase and force-push when you make the change - I can squash your commits upon merging.

stardiviner commented 5 years ago

Ok, I force pushed update. Thanks fore reviewing, @astahlman .

astahlman commented 5 years ago

@stardiviner I didn't have permission to push to your branch, so I created a throwaway PR and confirmed that it fixes the test: https://github.com/astahlman/ob-async/pull/54/files#r258313059

Want to apply that change to your branch so we can merge this one? (It's a one-line change).

stardiviner commented 5 years ago

Ok, I added it now, force pushed. Seems trigger Tracis CI again now.

stardiviner commented 5 years ago

Wow, all passed, clean green now. THanks @astahlman

astahlman commented 5 years ago

🎉 🎉 🎉